launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03483
[Merge] lp:~rvb/launchpad/change-perm-sync into lp:launchpad
Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/change-perm-sync into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #773261 in Launchpad itself: "The permission check for syncing packages on the differences pages (+localpackagediff) should be done on a per-package basis and not by checking lp.Edit on the series."
https://bugs.launchpad.net/launchpad/+bug/773261
For more details, see:
https://code.launchpad.net/~rvb/launchpad/change-perm-sync/+merge/59341
This branch fixes the permission check used to sync packages. It previously used lp.edit on the series and now checks upload permissions for every synced package.
= Details =
The tricky part is that this perm check cannot be performed by a security adapter because the check needs more information than what is available inside a security adapter.
- Added the permission check inside checkCopy (lib/lp/soyuz/scripts/packagecopier.py). The person requesting the sync had to be passed along the call stack.
- Removed the perm check against lp.edit on the series.
- Added new tests and refactored existing ones in lib/lp/registry/browser/tests/test_distroseries.py to avoid code duplication.
- Drive-by documentation fix: added documentation for parameter strict_component of checkUpload (lib/lp/soyuz/interfaces/archive.py).
- Drive-by style fix in lib/lp/registry/browser/tests/test_distroseries.py: renamed a few local variables (that where overriding an import), removed unused variables.
== Pre-implementation notes ==
Spoke with William Grant.
= Tests =
(modified tests)
./bin/test -cvv test_distroseries test_sync_error_nothing_selected
./bin/test -cvv test_distroseries test_sync_notification_on_success
./bin/test -cvv test_distroseries test_sync_error_invalid_selection
(added tests)
./bin/test -cvv test_distroseries test_canPerformSync_anon
./bin/test -cvv test_distroseries test_canPerformSync_non_anon
./bin/test -cvv test_distroseries test_sync_error_no_perm_dest_archive
./bin/test -cvv test_distroseries test_sync_success_perm_component
./bin/test -cvv test_distroseries test_sync_error_no_perm_component
./bin/test -cvv test_distroseries test_sync_success_not_yet_in_derived_series
= QA =
On DF: https://dogfood.launchpad.net/ubuntu/natty/+localpackagediffs
As anon user: the sync button should not be displayed.
As non privileged user: syncing packages should return an error.
As a user with perms on destination component A:
- syncing a package already existing in component B should fail.
- syncing a package already existing in component A should work.
- syncing a package not existing in the destination series should work.
(https://dogfood.launchpad.net/ubuntu/natty/+missingpackages)
--
https://code.launchpad.net/~rvb/launchpad/change-perm-sync/+merge/59341
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/change-perm-sync into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2011-04-26 14:40:00 +0000
+++ lib/lp/registry/browser/distroseries.py 2011-04-29 14:01:22 +0000
@@ -776,7 +776,7 @@
'selected_differences', sources, self.context.main_archive,
self.context, PackagePublishingPocket.RELEASE,
include_binaries=False, dest_url=series_url,
- dest_display_name=series_title):
+ dest_display_name=series_title, person=self.user):
# The copy worked so we can redirect back to the page to
# show the results.
self.next_url = self.request.URL
@@ -795,8 +795,7 @@
This method is used as a condition for the above sync action, as
well as directly in the template.
"""
- return (check_permission('launchpad.Edit', self.context) and
- self.cached_differences.batch.total() > 0)
+ return self.user is not None and self.cached_differences.batch.total() > 0
@property
def specified_name_filter(self):
=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py 2011-04-27 07:27:55 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-04-29 14:01:22 +0000
@@ -110,7 +110,7 @@
# Helper function to create a valid DSD.
distroseries = self.factory.makeDistroSeries(
parent_series=self.factory.makeDistroSeries())
- ds_diff = self.factory.makeDistroSeriesDifference(
+ self.factory.makeDistroSeriesDifference(
derived_series=distroseries, difference_type=difference_type)
return create_initialized_view(distroseries, '+index')
@@ -173,11 +173,11 @@
derived_series,
'+index',
principal=self.simple_user)
- html = view()
+ html_content = view()
self.assertEqual(
None, getFeatureFlag('soyuz.derived-series-ui.enabled'))
- self.assertThat(html, Not(portlet_header))
+ self.assertThat(html_content, Not(portlet_header))
def test_differences_portlet_all_differences(self):
# The difference portlet shows the differences with the parent
@@ -209,9 +209,9 @@
# XXX rvb 2011-04-12 bug=758649: LaunchpadTestRequest overrides
# self.features to NullFeatureController.
view.request.features = get_relevant_feature_controller()
- html = view()
+ html_content = view()
- self.assertThat(html, portlet_display)
+ self.assertThat(html_content, portlet_display)
def test_differences_portlet_no_differences(self):
# The difference portlet displays 'No differences' if there is no
@@ -235,9 +235,9 @@
# XXX rvb 2011-04-12 bug=758649: LaunchpadTestRequest overrides
# self.features to NullFeatureController.
view.request.features = get_relevant_feature_controller()
- html = view()
+ html_content = view()
- self.assertThat(html, portlet_display)
+ self.assertThat(html_content, portlet_display)
def test_differences_portlet_initialising(self):
# The difference portlet displays 'The series is initialising.' if
@@ -245,7 +245,7 @@
set_derived_series_ui_feature_flag(self)
derived_series = self._setupDifferences('deri', 'sid', 0, 0, 0)
job_source = getUtility(IInitialiseDistroSeriesJobSource)
- job = job_source.create(derived_series.parent, derived_series)
+ job_source.create(derived_series.parent, derived_series)
portlet_display = soupmatchers.HTMLContains(
soupmatchers.Tag(
'Derived series', 'h2',
@@ -263,10 +263,10 @@
# XXX rvb 2011-04-12 bug=758649: LaunchpadTestRequest overrides
# self.features to NullFeatureController.
view.request.features = get_relevant_feature_controller()
- html = view()
+ html_content = view()
self.assertTrue(derived_series.is_initialising)
- self.assertThat(html, portlet_display)
+ self.assertThat(html_content, portlet_display)
class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
@@ -1001,51 +1001,142 @@
self.assertContentEqual(
[resolved_diff], filtered_view.cached_differences.batch)
- def test_canPerformSync_non_editor(self):
- # Non-editors do not see options to sync.
+ def _setUpDSD(self, src_name='src-name', versions=None,
+ difference_type=None):
+ # Helper to create a derived series with fixed names and proper
+ # source package format selection along with a DSD.
+ parent_series = self.factory.makeDistroSeries(name='warty')
derived_series = self.factory.makeDistroSeries(
- name='derilucid', parent_series=self.factory.makeDistroSeries(
- name='lucid'))
+ distribution=self.factory.makeDistribution(name='deribuntu'),
+ name='derilucid', parent_series=parent_series)
+ self._set_source_selection(derived_series)
self.factory.makeDistroSeriesDifference(
- derived_series=derived_series)
-
+ source_package_name_str=src_name,
+ derived_series=derived_series, versions=versions,
+ difference_type=difference_type)
+ sourcepackagename = self.factory.getOrMakeSourcePackageName(
+ src_name)
set_derived_series_ui_feature_flag(self)
+ return derived_series, parent_series, sourcepackagename
+
+ def test_canPerformSync_anon(self):
+ # Anonymous users do not see options to sync.
+ derived_series, _, _ = self._setUpDSD()
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs')
+
+ self.assertFalse(view.canPerformSync())
+
+ def test_canPerformSync_non_anon(self):
+ # Logged-in users are presented with options to perform syncs.
+ # Note that a more fine-grained perm check is done on each
+ # synced package.
+ derived_series, _, _ = self._setUpDSD()
with person_logged_in(self.factory.makePerson()):
view = create_initialized_view(
derived_series, '+localpackagediffs')
- self.assertFalse(view.canPerformSync())
-
- def test_canPerformSync_editor(self):
- # Editors are presented with options to perform syncs.
- derived_series = self.factory.makeDistroSeries(
- name='derilucid', parent_series=self.factory.makeDistroSeries(
- name='lucid'))
- self.factory.makeDistroSeriesDifference(
- derived_series=derived_series)
-
- set_derived_series_ui_feature_flag(self)
- with person_logged_in(derived_series.owner):
- view = create_initialized_view(
- derived_series, '+localpackagediffs')
self.assertTrue(view.canPerformSync())
+ def _syncAndGetView(self, derived_series, person, sync_differences,
+ difference_type=None, view_name='+localpackagediffs'):
+ # A helper to get the POST'ed sync view.
+ with person_logged_in(person):
+ view = create_initialized_view(
+ derived_series, view_name,
+ method='POST', form={
+ 'field.selected_differences': sync_differences,
+ 'field.actions.sync': 'Sync',
+ })
+ return view
+
+ def test_sync_error_nothing_selected(self):
+ # An error is raised when a sync is requested without any selection.
+ derived_series, _, _ = self._setUpDSD()
+ view = self._syncAndGetView(derived_series, derived_series.owner, [])
+
+ self.assertEqual(1, len(view.errors))
+ self.assertEqual(
+ 'No differences selected.', view.errors[0])
+
+ def test_sync_error_invalid_selection(self):
+ # An error is raised when an invalid difference is selected.
+ derived_series, _, _ = self._setUpDSD('my-src-name')
+ view = self._syncAndGetView(
+ derived_series, derived_series.owner, ['some-other-name'])
+
+ self.assertEqual(2, len(view.errors))
+ self.assertEqual(
+ 'No differences selected.', view.errors[0])
+ self.assertEqual(
+ 'Invalid value', view.errors[1].error_name)
+
+ def test_sync_error_no_perm_dest_archive(self):
+ # A user without upload rights on the destination archive cannot
+ # sync packages.
+ derived_series, _, _ = self._setUpDSD('my-src-name')
+ view = self._syncAndGetView(
+ derived_series, self.factory.makePerson(), ['my-src-name'])
+
+ self.assertEqual(1, len(view.errors))
+ self.assertTrue(
+ "The signer of this package has no upload rights to this "
+ "distribution's primary archive" in view.errors[0])
+
+ def makePersonWithComponentPermission(self, archive, component=None):
+ person = self.factory.makePerson()
+ if component is None:
+ component = self.factory.makeComponent()
+ removeSecurityProxy(archive).newComponentUploader(
+ person, component)
+ return person, component
+
+ def test_sync_success_perm_component(self):
+ # A user with upload rights on the destination component
+ # can sync packages.
+ derived_series, parent_series, sourcepackagename = self._setUpDSD(
+ 'my-src-name')
+ person, _ = self.makePersonWithComponentPermission(
+ derived_series.main_archive,
+ derived_series.getSourcePackage(
+ sourcepackagename).latest_published_component)
+ view = self._syncAndGetView(
+ derived_series, person, ['my-src-name'])
+
+ self.assertEqual(0, len(view.errors))
+
+ def test_sync_error_no_perm_component(self):
+ # A user without upload rights on the destination component
+ # will get an error when he syncs packages to this component.
+ derived_series, parent_series, sourcepackagename = self._setUpDSD(
+ 'my-src-name')
+ person, another_component = self.makePersonWithComponentPermission(
+ derived_series.main_archive)
+ view = self._syncAndGetView(
+ derived_series, person, ['my-src-name'])
+
+ self.assertEqual(1, len(view.errors))
+ self.assertTrue(
+ "Signer is not permitted to upload to the "
+ "component" in view.errors[0])
+
def test_sync_notification_on_success(self):
- # Syncing one or more diffs results in a stub notification.
+ # A user with upload rights on the destination archive can
+ # sync packages. Notifications about the synced packages are
+ # displayed and the packages are copied inside the destination
+ # series.
versions = {
'base': '1.0',
'derived': '1.0derived1',
'parent': '1.0-1',
}
- parent_series = self.factory.makeDistroSeries(name='warty')
- derived_distro = self.factory.makeDistribution(name='deribuntu')
- derived_series = self.factory.makeDistroSeries(
- distribution=derived_distro, name='derilucid',
- parent_series=parent_series)
- self._set_source_selection(derived_series)
- difference = self.factory.makeDistroSeriesDifference(
- source_package_name_str='my-src-name',
- derived_series=derived_series, versions=versions)
+ derived_series, parent_series, sourcepackagename = self._setUpDSD(
+ 'my-src-name', versions=versions)
+
+ # Setup a user with upload rights.
+ person = self.factory.makePerson()
+ removeSecurityProxy(derived_series.main_archive).newPackageUploader(
+ person, sourcepackagename)
# The inital state is that 1.0-1 is not in the derived series.
pubs = derived_series.main_archive.getPublishedSources(
@@ -1054,16 +1145,8 @@
self.assertIs(None, pubs)
# Now, sync the source from the parent using the form.
- set_derived_series_ui_feature_flag(self)
- with person_logged_in(derived_series.owner):
- view = create_initialized_view(
- derived_series, '+localpackagediffs',
- method='POST', form={
- 'field.selected_differences': [
- difference.source_package_name.name,
- ],
- 'field.actions.sync': 'Sync',
- })
+ view = self._syncAndGetView(
+ derived_series, person, ['my-src-name'])
# The parent's version should now be in the derived series:
pub = derived_series.main_archive.getPublishedSources(
@@ -1086,52 +1169,31 @@
# 302 is a redirect back to the same page.
self.assertEqual(302, view.request.response.getStatus())
- def test_sync_error_nothing_selected(self):
- # An error is raised when a sync is requested without any selection.
- derived_series = self.factory.makeDistroSeries(
- name='derilucid', parent_series=self.factory.makeDistroSeries(
- name='lucid'))
- self.factory.makeDistroSeriesDifference(
- source_package_name_str='my-src-name',
- derived_series=derived_series)
-
- set_derived_series_ui_feature_flag(self)
- with person_logged_in(derived_series.owner):
- view = create_initialized_view(
- derived_series, '+localpackagediffs',
- method='POST', form={
- 'field.selected_differences': [],
- 'field.actions.sync': 'Sync',
- })
-
- self.assertEqual(1, len(view.errors))
- self.assertEqual(
- 'No differences selected.', view.errors[0])
-
- def test_sync_error_invalid_selection(self):
- # An error is raised when an invalid difference is selected.
- derived_series = self.factory.makeDistroSeries(
- name='derilucid', parent_series=self.factory.makeDistroSeries(
- name='lucid'))
- self._set_source_selection(derived_series)
- self.factory.makeDistroSeriesDifference(
- source_package_name_str='my-src-name',
- derived_series=derived_series)
-
- set_derived_series_ui_feature_flag(self)
- with person_logged_in(derived_series.owner):
- view = create_initialized_view(
- derived_series, '+localpackagediffs',
- method='POST', form={
- 'field.selected_differences': ['some-other-name'],
- 'field.actions.sync': 'Sync',
- })
-
- self.assertEqual(2, len(view.errors))
- self.assertEqual(
- 'No differences selected.', view.errors[0])
- self.assertEqual(
- 'Invalid value', view.errors[1].error_name)
+ def test_sync_success_not_yet_in_derived_series(self):
+ # If the package to sync does not exist yet in the derived series,
+ # upload right to any component inside the destination series will be
+ # enough to sync the package.
+ versions = {
+ 'parent': '1.0-1',
+ }
+ missing = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+ derived_series, parent_series, sourcepackagename = self._setUpDSD(
+ 'my-src-name', difference_type=missing, versions=versions)
+ person, another_component = self.makePersonWithComponentPermission(
+ derived_series.main_archive)
+ view = self._syncAndGetView(
+ derived_series, person, ['my-src-name'],
+ view_name='+missingpackages')
+
+ self.assertEqual(0, len(view.errors))
+ notifications = view.request.response.notifications
+ self.assertEqual(1, len(notifications))
+ self.assertEqual(
+ '<p>Packages copied to '
+ '<a href="http://launchpad.dev/deribuntu/derilucid";'
+ '>Derilucid</a>:</p>\n<ul>\n<li>my-src-name 1.0-1 in '
+ 'derilucid</li>\n</ul>',
+ notifications[0].message)
class TestDistroSeriesNeedsPackagesView(TestCaseWithFactory):
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2011-04-12 01:43:42 +0000
+++ lib/lp/soyuz/browser/archive.py 2011-04-29 14:01:22 +0000
@@ -1219,7 +1219,7 @@
def do_copy(self, sources_field_name, source_pubs, dest_archive,
dest_series, dest_pocket, include_binaries,
- dest_url=None, dest_display_name=None):
+ dest_url=None, dest_display_name=None, person=None):
"""Copy packages and add appropriate feedback to the browser page.
:param sources_field_name: The name of the form field to set errors
@@ -1236,13 +1236,16 @@
:param dest_display_name: The text to use for the dest_url link.
Defaults to the target archive's display name and will be
automatically escaped for inclusion in the output.
+ :param person: The (optional) person requesting the copy (if present,
+ will be used for the permission check).
:return: True if the copying worked, False otherwise.
"""
try:
copies = do_copy(
source_pubs, dest_archive, dest_series,
- dest_pocket, include_binaries)
+ dest_pocket, include_binaries, allow_delayed_copies=True,
+ person=person)
except CannotCopy, error:
messages = []
error_lines = str(error).splitlines()
=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py 2011-04-11 05:21:38 +0000
+++ lib/lp/soyuz/interfaces/archive.py 2011-04-29 14:01:22 +0000
@@ -616,6 +616,9 @@
:param component: The `Component` being uploaded to.
:param pocket: The `PackagePublishingPocket` of 'distroseries' being
uploaded to.
+ :param strict_component: True if access to the specific component for
+ the package is needed to upload to it. If False, then access to
+ any package will do.
:return: The reason for not being able to upload, None otherwise.
"""
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2011-04-11 05:38:23 +0000
+++ lib/lp/soyuz/model/archive.py 2011-04-29 14:01:22 +0000
@@ -1175,7 +1175,7 @@
strict_component)
def verifyUpload(self, person, sourcepackagename, component,
- distroseries, strict_component=True):
+ distroseries, strict_component=True):
"""See `IArchive`."""
if not self.enabled:
return ArchiveDisabled(self.displayname)
=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py 2011-04-11 14:34:55 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py 2011-04-29 14:01:22 +0000
@@ -28,10 +28,11 @@
from lp.app.errors import NotFoundError
from lp.buildmaster.enums import BuildStatus
from lp.soyuz.adapters.packagelocation import build_package_location
-from lp.soyuz.enums import ArchivePurpose
-from lp.soyuz.interfaces.archive import (
- CannotCopy,
+from lp.soyuz.enums import (
+ ArchivePurpose,
+ SourcePackageFormat,
)
+from lp.soyuz.interfaces.archive import CannotCopy
from lp.soyuz.interfaces.binarypackagebuild import BuildSetStatus
from lp.soyuz.interfaces.publishing import (
active_publishing_status,
@@ -44,7 +45,6 @@
IPackageUploadCustom,
IPackageUploadSet,
)
-from lp.soyuz.enums import SourcePackageFormat
from lp.soyuz.scripts.ftpmasterbase import (
SoyuzScript,
SoyuzScriptError,
@@ -374,7 +374,7 @@
"%s already exists in destination archive with "
"different contents." % lf.libraryfile.filename)
- def checkCopy(self, source, series, pocket):
+ def checkCopy(self, source, series, pocket, person=None):
"""Check if the source can be copied to the given location.
Check possible conflicting publications in the destination archive.
@@ -384,13 +384,34 @@
higher than any version of the same source present in the
destination suite (series + pocket).
+ If person is not None, check that this person has upload rights to
+ the destination (archive, component, pocket).
+
:param source: copy candidate, `ISourcePackagePublishingHistory`.
:param series: destination `IDistroSeries`.
:param pocket: destination `PackagePublishingPocket`.
+ :param person: requestor `IPerson`.
:raise CannotCopy when a copy is not allowed to be performed
containing the reason of the error.
"""
+ # If there is a requestor, check that he has upload permission
+ # into the destination (archive, component, pocket). This check
+ # is done here rather than in the security adapter because it
+ # requires more info than is available in the security adapter.
+ if person is not None:
+ sourcepackagename = source.sourcepackagerelease.sourcepackagename
+ destination_component = series.getSourcePackage(
+ sourcepackagename).latest_published_component
+ # If destination_component is not None, make sure the person
+ # has upload permission for this component. Otherwise, any upload
+ # permission on this archive will do.
+ reason = self.archive.checkUpload(
+ person, series, sourcepackagename, destination_component,
+ pocket, strict_component=(destination_component is not None))
+ if reason:
+ raise CannotCopy(reason)
+
if series not in self.archive.distribution.series:
raise CannotCopy(
"No such distro series %s in distribution %s." %
@@ -459,7 +480,7 @@
def do_copy(sources, archive, series, pocket, include_binaries=False,
- allow_delayed_copies=True):
+ allow_delayed_copies=True, person=None):
"""Perform the complete copy of the given sources incrementally.
Verifies if each copy can be performed using `CopyChecker` and
@@ -481,6 +502,7 @@
:param allow_delayed_copies: boolean indicating whether or not private
sources can be copied to public archives using delayed_copies.
Defaults to True, only set as False in the UnembargoPackage context.
+ :param: person: the requestor `IPerson`.
:raise CannotCopy when one or more copies were not allowed. The error
will contain the reason why each copy was denied.
@@ -500,7 +522,7 @@
else:
destination_series = series
try:
- copy_checker.checkCopy(source, destination_series, pocket)
+ copy_checker.checkCopy(source, destination_series, pocket, person)
except CannotCopy, reason:
errors.append("%s (%s)" % (source.displayname, reason))
continue
