launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25316
[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
