← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] ~pappacena/launchpad:mp-refs-privacy-change into launchpad:master

 

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:mp-refs-privacy-change into launchpad:master with ~pappacena/launchpad:create-mp-refs as a prerequisite.

Commit message:
Background job to add/remove merge proposal's virtual refs when a repository has its privacy changed

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/390940
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:mp-refs-privacy-change into launchpad:master.
diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index 898e645..4adc8d0 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -1111,6 +1111,11 @@
       provides="lp.code.interfaces.gitjob.IGitRepositoryModifiedMailJobSource">
     <allow interface="lp.code.interfaces.gitjob.IGitRepositoryModifiedMailJobSource" />
   </securedutility>
+  <securedutility
+      component="lp.code.model.gitjob.GitRepositoryVirtualRefsSyncJob"
+      provides="lp.code.interfaces.gitjob.IGitRepositoryVirtualRefsSyncJobSource">
+    <allow interface="lp.code.interfaces.gitjob.IGitRepositoryVirtualRefsSyncJobSource" />
+  </securedutility>
   <class class="lp.code.model.gitjob.GitRefScanJob">
     <allow interface="lp.code.interfaces.gitjob.IGitJob" />
     <allow interface="lp.code.interfaces.gitjob.IGitRefScanJob" />
@@ -1123,6 +1128,10 @@
     <allow interface="lp.code.interfaces.gitjob.IGitJob" />
     <allow interface="lp.code.interfaces.gitjob.IGitRepositoryModifiedMailJob" />
   </class>
+    <class class="lp.code.model.gitjob.GitRepositoryVirtualRefsSyncJob">
+    <allow interface="lp.code.interfaces.gitjob.IGitJob" />
+    <allow interface="lp.code.interfaces.gitjob.IGitRepositoryVirtualRefsSyncJob" />
+  </class>
 
   <lp:help-folder folder="help" name="+help-code" />
 
diff --git a/lib/lp/code/interfaces/gitjob.py b/lib/lp/code/interfaces/gitjob.py
index 4f31b19..7c3c038 100644
--- a/lib/lp/code/interfaces/gitjob.py
+++ b/lib/lp/code/interfaces/gitjob.py
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """GitJob interfaces."""
@@ -11,6 +11,8 @@ __all__ = [
     'IGitRefScanJobSource',
     'IGitRepositoryModifiedMailJob',
     'IGitRepositoryModifiedMailJobSource',
+    'IGitRepositoryVirtualRefsSyncJob',
+    'IGitRepositoryVirtualRefsSyncJobSource',
     'IReclaimGitRepositorySpaceJob',
     'IReclaimGitRepositorySpaceJobSource',
     ]
@@ -93,3 +95,16 @@ class IGitRepositoryModifiedMailJobSource(IJobSource):
         :param repository_delta: An `IGitRepositoryDelta` describing the
             changes.
         """
+
+
+class IGitRepositoryVirtualRefsSyncJob(IRunnableJob):
+    """A job to synchronize all MPs virtual refs related to this repository."""
+
+
+class IGitRepositoryVirtualRefsSyncJobSource(IJobSource):
+
+    def create(repository):
+        """Send email about repository modifications.
+
+        :param repository: The `IGitRepository` that needs sync.
+        """
diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 8a29a99..bd804a8 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -1247,7 +1247,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             self.target_git_repository.getInternalPath(),
             GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))]
 
-    def copyGitHostingVirtualRefs(self):
+    def copyGitHostingVirtualRefs(self, logger=logger):
         """Requests virtual refs copy operations on GitHosting in order to
         keep them up-to-date with current MP's state.
 
@@ -1257,10 +1257,10 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             hosting_client = getUtility(IGitHostingClient)
             hosting_client.copyRefs(
                 self.source_git_repository.getInternalPath(),
-                copy_operations)
+                copy_operations, logger=logger)
         return copy_operations
 
-    def deleteGitHostingVirtualRefs(self, except_refs=None):
+    def deleteGitHostingVirtualRefs(self, except_refs=None, logger=None):
         """Deletes on git code hosting service all virtual refs, except
         those ones in the given list."""
         if self.source_git_ref is None:
@@ -1278,14 +1278,16 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
         if ref not in (except_refs or []):
             hosting_client = getUtility(IGitHostingClient)
             hosting_client.deleteRef(
-                self.target_git_repository.getInternalPath(), ref)
+                self.target_git_repository.getInternalPath(), ref,
+                logger=logger)
 
-    def syncGitHostingVirtualRefs(self):
+    def syncGitHostingVirtualRefs(self, logger=None):
         """Requests all copies and deletion of virtual refs to make git code
         hosting in sync with this MP."""
-        operations = self.copyGitHostingVirtualRefs()
+        operations = self.copyGitHostingVirtualRefs(logger=logger)
         copied_refs = [i.target_ref for i in operations]
-        self.deleteGitHostingVirtualRefs(except_refs=copied_refs)
+        self.deleteGitHostingVirtualRefs(
+            except_refs=copied_refs, logger=logger)
 
     def scheduleDiffUpdates(self, return_jobs=True):
         """See `IBranchMergeProposal`."""
diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py
index 3c041da..a18d28e 100644
--- a/lib/lp/code/model/gitjob.py
+++ b/lib/lp/code/model/gitjob.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -45,6 +45,8 @@ from lp.code.interfaces.gitjob import (
     IGitRefScanJobSource,
     IGitRepositoryModifiedMailJob,
     IGitRepositoryModifiedMailJobSource,
+    IGitRepositoryVirtualRefsSyncJob,
+    IGitRepositoryVirtualRefsSyncJobSource,
     IReclaimGitRepositorySpaceJob,
     IReclaimGitRepositorySpaceJobSource,
     )
@@ -100,6 +102,13 @@ class GitJobType(DBEnumeratedType):
         modifications.
         """)
 
+    SYNC_MP_VIRTUAL_REFS = DBItem(3, """
+        Sync merge proposals virtual refs
+
+        This job runs against a repository to synchronize the virtual refs
+        from all merge proposals related to this repository.
+        """)
+
 
 @implementer(IGitJob)
 class GitJob(StormBase):
@@ -404,3 +413,35 @@ class GitRepositoryModifiedMailJob(GitJobDerived):
     def run(self):
         """See `IGitRepositoryModifiedMailJob`."""
         self.getMailer().sendAll()
+
+
+@implementer(IGitRepositoryVirtualRefsSyncJob)
+@provider(IGitRepositoryVirtualRefsSyncJobSource)
+class GitRepositoryVirtualRefsSyncJob(GitJobDerived):
+    """A Job that scans a Git repository for its current list of references."""
+    class_job_type = GitJobType.SYNC_MP_VIRTUAL_REFS
+
+    max_retries = 5
+
+    retry_error_types = tuple()
+
+    config = config.IGitRepositoryVirtualRefsSyncJobSource
+
+    @classmethod
+    def create(cls, repository):
+        metadata = {}
+        git_job = GitJob(repository, cls.class_job_type, metadata)
+        job = cls(git_job)
+        job.celeryRunOnCommit()
+        return job
+
+    def run(self):
+        log.info(
+            "Starting to re-sync virtual refs from repository %s",
+            self.repository)
+        for mp in self.repository.landing_targets:
+            log.info("Re-syncing virtual refs from MP %s", mp)
+            mp.syncGitHostingVirtualRefs(logger=log)
+        log.info(
+            "Finished re-syncing virtual refs from repository %s",
+            self.repository)
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 875d980..f9958c8 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -116,7 +116,10 @@ from lp.code.interfaces.gitcollection import (
     IGitCollection,
     )
 from lp.code.interfaces.githosting import IGitHostingClient
-from lp.code.interfaces.gitjob import IGitRefScanJobSource
+from lp.code.interfaces.gitjob import (
+    IGitRefScanJobSource,
+    IGitRepositoryVirtualRefsSyncJobSource,
+    )
 from lp.code.interfaces.gitlookup import IGitLookup
 from lp.code.interfaces.gitnamespace import (
     get_git_namespace,
@@ -865,6 +868,7 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
             raise CannotChangeInformationType("Forbidden by project policy.")
         # XXX cjwatson 2019-03-29: Check privacy rules on snaps that use
         # this repository.
+        was_private = self.private
         self.information_type = information_type
         self._reconcileAccess()
         if (information_type in PRIVATE_INFORMATION_TYPES and
@@ -882,6 +886,12 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         # subscriptions.
         getUtility(IRemoveArtifactSubscriptionsJobSource).create(user, [self])
 
+        # If privacy changed, we need to re-sync all virtual refs from
+        # all MPs to avoid disclosing private code, or to add the virtual
+        # refs to the now public code.
+        if was_private != self.private:
+            getUtility(IGitRepositoryVirtualRefsSyncJobSource).create(self)
+
     def setName(self, new_name, user):
         """See `IGitRepository`."""
         self.namespace.moveRepository(self, user, new_name=new_name)
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 54cf516..277650c 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -285,7 +285,7 @@ class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory):
         self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
         args, kwargs = self.hosting_fixture.copyRefs.calls[0]
         arg_path, arg_copy_operations = args
-        self.assertEqual({}, kwargs)
+        self.assertEqual({'logger': None}, kwargs)
         self.assertEqual(mp.source_git_repository.getInternalPath(), arg_path)
         self.assertEqual(1, len(arg_copy_operations))
         self.assertThat(arg_copy_operations[0], MatchesStructure(
@@ -357,7 +357,7 @@ class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory):
         # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created.
         self.assertEqual(1, self.hosting_fixture.copyRefs.call_count)
         args, kwargs = self.hosting_fixture.copyRefs.calls[0]
-        self.assertEquals({}, kwargs)
+        self.assertEquals({'logger': None}, kwargs)
         self.assertEqual(args[0], source_repo.getInternalPath())
         self.assertEqual(1, len(args[1]))
         self.assertThat(args[1][0], MatchesStructure(
@@ -384,7 +384,7 @@ class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory):
         self.assertEqual(0, self.hosting_fixture.copyRefs.call_count)
         self.assertEqual(1, self.hosting_fixture.deleteRef.call_count)
         args, kwargs = self.hosting_fixture.deleteRef.calls[0]
-        self.assertEqual({}, kwargs)
+        self.assertEqual({'logger': None}, kwargs)
         self.assertEqual(
             (target_repo.getInternalPath(), "refs/merge/%s/head" % mp.id),
             args)
@@ -1658,7 +1658,7 @@ class TestBranchMergeProposalDeletion(TestCaseWithFactory):
         args = hosting_fixture.deleteRef.calls[0]
         self.assertEqual((
             (proposal.target_git_repository.getInternalPath(),
-             'refs/merge/%s/head' % proposal.id), {}), args)
+             'refs/merge/%s/head' % proposal.id), {'logger': None}), args)
 
 
 class TestBranchMergeProposalBugs(WithVCSScenarios, TestCaseWithFactory):
diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index 5964944..cc6ac49 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `GitJob`s."""
@@ -16,6 +16,7 @@ import hashlib
 from fixtures import FakeLogger
 from lazr.lifecycle.snapshot import Snapshot
 import pytz
+from storm.store import Store
 from testtools.matchers import (
     ContainsDict,
     Equals,
@@ -24,9 +25,11 @@ from testtools.matchers import (
     MatchesStructure,
     )
 import transaction
+from zope.component import getUtility
 from zope.interface import providedBy
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.enums import InformationType
 from lp.code.adapters.gitrepository import GitRepositoryDelta
 from lp.code.enums import (
     GitGranteeType,
@@ -38,8 +41,10 @@ from lp.code.interfaces.branchmergeproposal import (
 from lp.code.interfaces.gitjob import (
     IGitJob,
     IGitRefScanJob,
+    IGitRepositoryVirtualRefsSyncJobSource,
     IReclaimGitRepositorySpaceJob,
     )
+from lp.code.interfaces.gitrepository import GIT_CREATE_MP_VIRTUAL_REF
 from lp.code.model.gitjob import (
     describe_repository_delta,
     GitJob,
@@ -484,5 +489,35 @@ class TestDescribeRepositoryDelta(TestCaseWithFactory):
             snapshot, repository)
 
 
+class TestGitRepositoryVirtualRefsSyncJob(TestCaseWithFactory):
+    """Tests for `GitRepositoryVirtualRefsSyncJob`."""
+
+    layer = ZopelessDatabaseLayer
+
+    def test_changing_repo_to_private_deletes_refs(self):
+        self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"}))
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        mp = self.factory.makeBranchMergeProposalForGit()
+        source_repo = mp.source_git_repository
+        target_repo = mp.target_git_repository
+        source_repo.transitionToInformationType(
+            InformationType.PRIVATESECURITY, source_repo.owner, False)
+        Store.of(source_repo).flush()
+
+        hosting_fixture.copyRefs.resetCalls()
+        hosting_fixture.deleteRef.resetCalls()
+        with dbuser("branchscanner"):
+            job_set = JobRunner.fromReady(
+                getUtility(IGitRepositoryVirtualRefsSyncJobSource))
+            job_set.runAll()
+            self.assertEqual(1, len(job_set.completed_jobs))
+
+        self.assertEqual(0, hosting_fixture.copyRefs.call_count)
+        self.assertEqual(1, hosting_fixture.deleteRef.call_count)
+        args, kwargs = hosting_fixture.deleteRef.calls[0]
+        self.assertEqual(
+            (target_repo.getInternalPath(), 'refs/merge/%s/head' % mp.id),
+            args)
+
 # XXX cjwatson 2015-03-12: We should test that the jobs work via Celery too,
 # but that isn't feasible until we have a proper turnip fixture.
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index b819929..6a9fa20 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -18,6 +18,7 @@ from datetime import (
 import email
 from functools import partial
 import hashlib
+import itertools
 import json
 
 from breezy import urlutils
@@ -89,6 +90,7 @@ from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
 from lp.code.interfaces.gitjob import (
     IGitRefScanJobSource,
     IGitRepositoryModifiedMailJobSource,
+    IGitRepositoryVirtualRefsSyncJobSource,
     )
 from lp.code.interfaces.gitlookup import IGitLookup
 from lp.code.interfaces.gitnamespace import (
@@ -148,6 +150,7 @@ from lp.registry.interfaces.personociproject import IPersonOCIProjectFactory
 from lp.registry.interfaces.personproduct import IPersonProductFactory
 from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
 from lp.services.authserver.xmlrpc import AuthServerAPIView
+from lp.services.compat import mock
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.interfaces import IStore
@@ -182,6 +185,7 @@ from lp.testing import (
     verifyObject,
     )
 from lp.testing.dbuser import dbuser
+from lp.testing.fixture import ZopeUtilityFixture
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -4663,3 +4667,61 @@ class TestGitRepositoryMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
                 ["Caveat check for '%s' failed." %
                  find_caveats_by_name(macaroon2, "lp.expires")[0].caveat_id],
                 issuer, macaroon2, repository, user=repository.owner)
+
+
+class TestGitRepositoryPrivacyChangeSyncVirtualRefs(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def assertChangePrivacyTriggersSync(
+            self, from_list, to_list, should_trigger_sync=True):
+        """Runs repository.transitionToInformationType from every item in
+        `from_list` to each item in `to_list`, and checks if the virtual
+        refs sync was triggered or not, depending on `should_trigger_sync`."""
+        sync_job = mock.Mock()
+        self.useFixture(ZopeUtilityFixture(
+            sync_job, IGitRepositoryVirtualRefsSyncJobSource))
+
+        admin = self.factory.makeAdministrator()
+        login_person(admin)
+        for from_type, to_type in itertools.product(from_list, to_list):
+            if from_type == to_type:
+                continue
+            repository = self.factory.makeGitRepository()
+            naked_repo = removeSecurityProxy(repository)
+            naked_repo.information_type = from_type
+            # Skip access policy reconciliation.
+            naked_repo._reconcileAccess = mock.Mock()
+            naked_repo.transitionToInformationType(to_type, admin, False)
+
+            if should_trigger_sync:
+                sync_job.create.assert_called_with(repository)
+            else:
+                self.assertEqual(
+                    0, sync_job.create.call_count,
+                    "Changing from %s to %s should't trigger vrefs sync"
+                    % (from_type, to_type))
+                sync_job.reset_mock()
+
+    def test_setting_repo_public_triggers_ref_sync_job(self):
+        self.assertChangePrivacyTriggersSync(
+            PRIVATE_INFORMATION_TYPES,
+            PUBLIC_INFORMATION_TYPES,
+            should_trigger_sync=True)
+
+    def test_setting_repo_private_triggers_ref_sync_job(self):
+        self.assertChangePrivacyTriggersSync(
+            PUBLIC_INFORMATION_TYPES,
+            PRIVATE_INFORMATION_TYPES,
+            should_trigger_sync=True)
+
+    def test_keeping_repo_private_dont_trigger_ref_sync_job(self):
+        self.assertChangePrivacyTriggersSync(
+            PRIVATE_INFORMATION_TYPES,
+            PRIVATE_INFORMATION_TYPES,
+            should_trigger_sync=False)
+
+    def test_keeping_repo_public_dont_trigger_ref_sync_job(self):
+        self.assertChangePrivacyTriggersSync(
+            PUBLIC_INFORMATION_TYPES,
+            PUBLIC_INFORMATION_TYPES,
+            should_trigger_sync=False)
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index bb8afcd..3921da2 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -1828,6 +1828,10 @@ module: lp.code.interfaces.gitjob
 dbuser: send-branch-mail
 crontab_group: MAIN
 
+[IGitRepositoryVirtualRefsSyncJobSource]
+module: lp.code.interfaces.gitjob
+dbuser: branchscanner
+
 [IInitializeDistroSeriesJobSource]
 module: lp.soyuz.interfaces.distributionjob
 dbuser: initializedistroseries
diff --git a/lib/lp/testing/fakemethod.py b/lib/lp/testing/fakemethod.py
index 4bba8d3..b4895ce 100644
--- a/lib/lp/testing/fakemethod.py
+++ b/lib/lp/testing/fakemethod.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -57,3 +57,6 @@ class FakeMethod:
     def extract_kwargs(self):
         """Return just the calls' keyword-arguments dicts."""
         return [kwargs for args, kwargs in self.calls]
+
+    def resetCalls(self):
+        self.calls = []

Follow ups