launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14051
[Merge] lp:~stevenk/launchpad/local-codeimports-bad into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/local-codeimports-bad into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #820069 in Launchpad itself: "BadUrlLaunchpad raised mirroring a branch"
https://bugs.launchpad.net/launchpad/+bug/820069
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/local-codeimports-bad/+merge/133183
Deny creation of a code import if it for a launchpad.net URL. I have refactored the portion of the validation method for both ProductSeries:+setbranch and CodeImport:+new-import (and CodeImport:+edit-import) that check the URL passed it.
I have also forced this branch to net-neutral by performing a bunch of whitespace cleanup as well as defining a new code enum, much like the same thing we did with PUBLIC_INFORMATION_TYPES and friends while on Disclosure.
This branch does not completely close the bug, since it can still occur after this lands. The next step is to clean up the production data.
--
https://code.launchpad.net/~stevenk/launchpad/local-codeimports-bad/+merge/133183
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/local-codeimports-bad into lp:launchpad.
=== modified file 'lib/lp/code/browser/codeimport.py'
--- lib/lp/code/browser/codeimport.py 2012-10-09 01:07:52 +0000
+++ lib/lp/code/browser/codeimport.py 2012-11-07 05:18:19 +0000
@@ -12,8 +12,10 @@
'CodeImportSetBreadcrumb',
'CodeImportSetNavigation',
'CodeImportSetView',
+ 'validate_import_url',
]
+from urlparse import urlparse
from BeautifulSoup import BeautifulSoup
from lazr.restful.interface import (
@@ -49,6 +51,7 @@
BranchSubscriptionNotificationLevel,
CodeImportReviewStatus,
CodeReviewNotificationLevel,
+ NON_CVS_RCS_TYPES,
RevisionControlSystems,
)
from lp.code.errors import BranchExists
@@ -167,10 +170,7 @@
def setSecondaryFieldError(self, field, error):
"""Set the field error only if there isn't an error already."""
- if self.getFieldError(field):
- # Leave this one as it is often required or a validator error.
- pass
- else:
+ if not self.getFieldError(field):
self.setFieldError(field, error)
def _validateCVS(self, cvs_root, cvs_module, existing_import=None):
@@ -201,16 +201,9 @@
self.setSecondaryFieldError(
field_name, 'Enter the URL of a foreign VCS branch.')
else:
- code_import = getUtility(ICodeImportSet).getByURL(url)
- if (code_import is not None and
- code_import != existing_import):
- self.setFieldError(
- field_name,
- structured("""
- This foreign branch URL is already specified for
- the imported branch <a href="%s">%s</a>.""",
- canonical_url(code_import.branch),
- code_import.branch.unique_name))
+ reason = validate_import_url(url, existing_import)
+ if reason:
+ self.setFieldError(field_name, reason)
class NewCodeImportForm(Interface):
@@ -540,12 +533,8 @@
# fields, and vice versa.
if self.code_import.rcs_type == RevisionControlSystems.CVS:
self.form_fields = self.form_fields.omit('url')
- elif self.code_import.rcs_type in (RevisionControlSystems.SVN,
- RevisionControlSystems.BZR_SVN,
- RevisionControlSystems.GIT,
- RevisionControlSystems.BZR):
- self.form_fields = self.form_fields.omit(
- 'cvs_root', 'cvs_module')
+ elif self.code_import.rcs_type in NON_CVS_RCS_TYPES:
+ self.form_fields = self.form_fields.omit('cvs_root', 'cvs_module')
else:
raise AssertionError('Unknown rcs_type for code import.')
@@ -575,10 +564,7 @@
self._validateCVS(
data.get('cvs_root'), data.get('cvs_module'),
self.code_import)
- elif self.code_import.rcs_type in (RevisionControlSystems.SVN,
- RevisionControlSystems.BZR_SVN,
- RevisionControlSystems.GIT,
- RevisionControlSystems.BZR):
+ elif self.code_import.rcs_type in NON_CVS_RCS_TYPES:
self._validateURL(data.get('url'), self.code_import)
else:
raise AssertionError('Unknown rcs_type for code import.')
@@ -593,3 +579,19 @@
def machines(self):
"""Get the machines, sorted alphabetically by hostname."""
return getUtility(ICodeImportMachineSet).getAll()
+
+
+def validate_import_url(url, existing_import=None):
+ """Validate the given import URL."""
+ if urlparse(url).netloc.endswith('launchpad.net'):
+ return (
+ "You can not create imports for branches that are hosted by "
+ "Launchpad.")
+ code_import = getUtility(ICodeImportSet).getByURL(url)
+ if code_import is not None:
+ if existing_import and code_import == existing_import:
+ return None
+ return structured(
+ "This foreign branch URL is already specified for the imported "
+ "branch <a href='%s'>%s</a>.", canonical_url(code_import.branch),
+ code_import.branch.unique_name)
=== modified file 'lib/lp/code/enums.py'
--- lib/lp/code/enums.py 2012-10-11 04:14:37 +0000
+++ lib/lp/code/enums.py 2012-11-07 05:18:19 +0000
@@ -20,6 +20,7 @@
'CodeImportReviewStatus',
'CodeReviewNotificationLevel',
'CodeReviewVote',
+ 'NON_CVS_RCS_TYPES',
'RevisionControlSystems',
'UICreatableBranchType',
]
@@ -856,3 +857,7 @@
The reviewer needs more information before making a decision.
""")
+
+NON_CVS_RCS_TYPES = (
+ RevisionControlSystems.SVN, RevisionControlSystems.BZR_SVN,
+ RevisionControlSystems.GIT, RevisionControlSystems.BZR)
=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py 2012-10-10 22:18:41 +0000
+++ lib/lp/code/model/codeimport.py 2012-11-07 05:18:19 +0000
@@ -38,6 +38,7 @@
CodeImportJobState,
CodeImportResultStatus,
CodeImportReviewStatus,
+ NON_CVS_RCS_TYPES,
RevisionControlSystems,
)
from lp.code.errors import (
@@ -71,8 +72,7 @@
_defaultOrder = ['id']
date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
- branch = ForeignKey(dbName='branch', foreignKey='Branch',
- notNull=True)
+ branch = ForeignKey(dbName='branch', foreignKey='Branch', notNull=True)
registrant = ForeignKey(
dbName='registrant', foreignKey='Person',
storm_validator=validate_public_person, notNull=True)
@@ -243,10 +243,7 @@
if rcs_type == RevisionControlSystems.CVS:
assert cvs_root is not None and cvs_module is not None
assert url is None
- elif rcs_type in (RevisionControlSystems.SVN,
- RevisionControlSystems.BZR_SVN,
- RevisionControlSystems.GIT,
- RevisionControlSystems.BZR):
+ elif rcs_type in NON_CVS_RCS_TYPES:
assert cvs_root is None and cvs_module is None
assert url is not None
else:
=== modified file 'lib/lp/code/stories/codeimport/xx-create-codeimport.txt'
--- lib/lp/code/stories/codeimport/xx-create-codeimport.txt 2012-10-09 01:07:52 +0000
+++ lib/lp/code/stories/codeimport/xx-create-codeimport.txt 2012-11-07 05:18:19 +0000
@@ -97,6 +97,19 @@
at http://user:password@xxxxxxxxxxxxxxx/firefox/trunk.
The next import is scheduled to run
as soon as possible.
+
+Specifing a Launchpad URL results in an error.
+
+ >>> browser.open("http://code.launchpad.dev/+code-imports/+new";)
+ >>> browser.getControl('Branch Name').value = "invalid"
+ >>> browser.getControl('Branch URL', index=0).value = (
+ ... "http://bazaar.launchpad.net/firefox/trunk";)
+ >>> browser.getControl('Project').value = "firefox"
+ >>> browser.getControl('Request Import').click()
+ >>> for message in get_feedback_messages(browser.contents):
+ ... print extract_text(message)
+ There is 1 error.
+ You can not create imports for branches that are hosted by Launchpad.
Requesting a Subversion import
==============================
=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py 2012-10-16 15:12:09 +0000
+++ lib/lp/registry/browser/productseries.py 2012-11-07 05:18:19 +0000
@@ -86,6 +86,7 @@
from lp.bugs.interfaces.bugtask import IBugTaskSet
from lp.code.browser.branch import BranchNameValidationMixin
from lp.code.browser.branchref import BranchRef
+from lp.code.browser.codeimport import validate_import_url
from lp.code.enums import (
BranchType,
RevisionControlSystems,
@@ -660,10 +661,7 @@
return 'Edit %s %s series' % (
self.context.product.displayname, self.context.name)
- @property
- def page_title(self):
- """The page title."""
- return self.label
+ page_title = label
def validate(self, data):
"""See `LaunchpadFormView`."""
@@ -697,10 +695,7 @@
return 'Delete %s %s series' % (
self.context.product.displayname, self.context.name)
- @property
- def page_title(self):
- """The page title."""
- return self.label
+ page_title = label
@cachedproperty
def milestones(self):
@@ -906,8 +901,7 @@
"""Validate data for link-lp-bzr case."""
if 'branch_location' not in data:
self.setFieldError(
- 'branch_location',
- 'The branch location must be set.')
+ 'branch_location', 'The branch location must be set.')
def _validateImportExternal(self, data):
"""Validate data for import external case."""
@@ -925,16 +919,9 @@
self.setFieldError(
'repo_url', 'You must set the external repository URL.')
else:
- # Ensure this URL has not been imported before.
- code_import = getUtility(ICodeImportSet).getByURL(repo_url)
- if code_import is not None:
- self.setFieldError(
- 'repo_url',
- structured("""
- This foreign branch URL is already specified for
- the imported branch <a href="%s">%s</a>.""",
- canonical_url(code_import.branch),
- code_import.branch.unique_name))
+ reason = validate_import_url(repo_url)
+ if reason:
+ self.setFieldError('repo_url', reason)
# RCS type is mandatory.
# This condition should never happen since an initial value is set.
@@ -945,19 +932,15 @@
'You must specify the type of RCS for the remote host.')
elif rcs_type == RevisionControlSystems.CVS:
if 'cvs_module' not in data:
- self.setFieldError(
- 'cvs_module',
- 'The CVS module must be set.')
+ self.setFieldError('cvs_module', 'The CVS module must be set.')
self._validateBranch(data)
def _validateBranch(self, data):
"""Validate that branch name and owner are set."""
if 'branch_name' not in data:
- self.setFieldError(
- 'branch_name', 'The branch name must be set.')
+ self.setFieldError('branch_name', 'The branch name must be set.')
if 'branch_owner' not in data:
- self.setFieldError(
- 'branch_owner', 'The branch owner must be set.')
+ self.setFieldError('branch_owner', 'The branch owner must be set.')
def _setRequired(self, names, value):
"""Mark the widget field as optional."""
@@ -1030,25 +1013,19 @@
branch_type = data.get('branch_type')
if branch_type == LINK_LP_BZR:
branch_location = data.get('branch_location')
+ self.context.branch = branch_location
if branch_location != self.context.branch:
- self.context.branch = branch_location
# Request an initial upload of translation files.
getUtility(IRosettaUploadJobSource).create(
self.context.branch, NULL_REVISION)
- else:
- self.context.branch = branch_location
self.request.response.addInfoNotification(
'Series code location updated.')
else:
branch_name = data.get('branch_name')
branch_owner = data.get('branch_owner')
- # Import or mirror an external branch.
if branch_type == IMPORT_EXTERNAL:
- # Either create an externally hosted bzr branch
- # (a.k.a. 'mirrored') or create a new code import.
rcs_type = data.get('rcs_type')
- # We need to create an import request.
if rcs_type == RevisionControlSystems.CVS:
cvs_root = data.get('repo_url')
cvs_module = data.get('cvs_module')
@@ -1069,8 +1046,7 @@
cvs_root=cvs_root,
cvs_module=cvs_module)
except BranchExists as e:
- self._setBranchExists(
- e.existing_branch, 'branch_name')
+ self._setBranchExists(e.existing_branch, 'branch_name')
self.errors_in_action = True
# Abort transaction. This is normally handled
# by LaunchpadFormView, but we are already in
@@ -1126,13 +1102,11 @@
@action(_('Update'), name='update')
def update_action(self, action, data):
"""Update the branch attribute."""
+ self.updateContextFromData(data)
if data['branch'] != self.context.branch:
- self.updateContextFromData(data)
# Request an initial upload of translation files.
getUtility(IRosettaUploadJobSource).create(
self.context.branch, NULL_REVISION)
- else:
- self.updateContextFromData(data)
self.request.response.addInfoNotification(
'Series code location updated.')
@@ -1177,8 +1151,7 @@
class ProductSeriesRdfView(BaseRdfView):
"""A view that sets its mime-type to application/rdf+xml"""
- template = ViewPageTemplateFile(
- '../templates/productseries-rdf.pt')
+ template = ViewPageTemplateFile('../templates/productseries-rdf.pt')
@property
def filename(self):
=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt 2012-10-09 01:07:52 +0000
+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt 2012-11-07 05:18:19 +0000
@@ -138,7 +138,6 @@
('repo_url'...The URI scheme "bzr+ssh" is not allowed. Only URIs with
the following schemes may be used: bzr, http, https'))
-
A correct URL is accepted.
>>> form = {
@@ -161,6 +160,17 @@
blazer-branch
>>> series.branch.registrant.name == driver.name
True
+
+External imports can not use an Launchpad URL.
+
+ >>> form['field.repo_url'] = 'http://bazaar.launchpad.net/firefox/foo'
+ >>> form['field.branch_name'] = 'chevette-branch'
+ >>> view = create_initialized_view(
+ ... series, name='+setbranch', principal=driver, form=form)
+ >>> transaction.commit()
+ >>> for error in view.errors:
+ ... print error
+ You can not create imports for branches that are hosted by Launchpad.
Git branches cannnot use svn.
@@ -303,7 +313,7 @@
<BLANKLINE>
This foreign branch URL is already specified for
the imported branch
- <a href="http://.../chevy/chevette-branch";>~.../chevy/chevette-branch</a>.
+ <a href='http://.../chevy/chevette-branch'>~.../chevy/chevette-branch</a>.
>>> for notification in view.request.response.notifications:
... print notification.message
@@ -348,7 +358,7 @@
... print error
<BLANKLINE>
This foreign branch URL is already specified for the imported branch
- <a href="http://.../blazer-branch";>...</a>.
+ <a href='http://.../blazer-branch'>...</a>.
>>> print view.errors_in_action
False
>>> print view.next_url
Follow ups
