launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03708
[Merge] lp:~julian-edwards/launchpad/aa-only-blacklist-bug-782222 into lp:launchpad/db-devel
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/aa-only-blacklist-bug-782222 into lp:launchpad/db-devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/aa-only-blacklist-bug-782222/+merge/62001
This branch introduces an lp.admin permission for DistroSeriesDifference and ensures that blacklisting and unblacklisting require it.
lp.admin here is either an archive admin or an LP admin.
In addition, the display of the blacklisting radio button widget is also dependent on this permission. Since the view's condition property "show_edit_options" was also shared with adding comments, I've split that out to its own property.
Everything else is fairly boilerplate.
--
https://code.launchpad.net/~julian-edwards/launchpad/aa-only-blacklist-bug-782222/+merge/62001
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/aa-only-blacklist-bug-782222 into lp:launchpad/db-devel.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2011-05-17 03:07:32 +0000
+++ lib/canonical/launchpad/security.py 2011-05-23 17:12:36 +0000
@@ -112,6 +112,7 @@
from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.distroseriesparent import IDistroSeriesParent
from lp.registry.interfaces.distroseriesdifference import (
+ IDistroSeriesDifferenceAdmin,
IDistroSeriesDifferenceEdit,
)
from lp.registry.interfaces.entitlement import IEntitlement
@@ -987,6 +988,20 @@
usedfor = ICountry
+class AdminDistroSeriesDifference(AuthorizationBase):
+ """You need to be an archive admin or LP admin to get lp.Admin."""
+ permission = 'launchpad.Admin'
+ usedfor = IDistroSeriesDifferenceAdmin
+
+ def checkAuthenticated(self, user):
+ # Archive admin is done by component, so here we just
+ # see if the user has that permission on any components
+ # at all.
+ archive = self.obj.derived_series.main_archive
+ return bool(
+ archive.getComponentsForQueueAdmin(user.person)) or user.in_admin
+
+
class EditDistroSeriesDifference(AuthorizationBase):
"""Anyone with lp.View on the distribution can edit a DSD."""
permission = 'launchpad.Edit'
=== modified file 'lib/lp/registry/browser/distroseriesdifference.py'
--- lib/lp/registry/browser/distroseriesdifference.py 2011-05-18 18:39:03 +0000
+++ lib/lp/registry/browser/distroseriesdifference.py 2011-05-23 17:12:36 +0000
@@ -52,6 +52,7 @@
IComment,
IConversation,
)
+from lp.services.propertycache import cachedproperty
class DistroSeriesDifferenceNavigation(Navigation):
@@ -148,16 +149,26 @@
DistroSeriesDifferenceDisplayComment(comment) for
comment in comments]
- @property
+ @cachedproperty
def can_request_diffs(self):
"""Does the user have permission to request diff calculation?"""
return check_permission('launchpad.Edit', self.context)
- @property
- def show_edit_options(self):
- """Only show the options if an editor requests via JS."""
+ @cachedproperty
+ def show_add_comment(self):
+ """Only show the 'Add comment' if an editor requests via JS."""
return self.request.is_ajax and self.can_request_diffs
+ @cachedproperty
+ def show_blacklist_options(self):
+ """Should we show the blacklisting (ignore) radio widget options.
+
+ Only show the options if an editor requests via JS and the user
+ is an archive admin.
+ """
+ return self.request.is_ajax and check_permission(
+ 'launchpad.Admin', self.context)
+
@property
def display_diffs(self):
"""Only show diffs if there's a base version."""
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2011-05-12 12:09:17 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2011-05-23 17:12:36 +0000
@@ -142,34 +142,51 @@
self.assertIs(None, view.binary_summaries)
def test_show_edit_options_non_ajax(self):
- # Blacklist options are not shown for non-ajax requests.
+ # Blacklist options and "Add comment" are not shown for non-ajax
+ # requests.
ds_diff = self.factory.makeDistroSeriesDifference()
# Without JS, even editors don't see blacklist options.
with person_logged_in(self.factory.makePerson()):
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra')
- self.assertFalse(view.show_edit_options)
+ self.assertFalse(view.show_add_comment)
+ self.assertFalse(view.show_blacklist_options)
def test_show_edit_options_editor(self):
- # Blacklist options are shown if requested by an editor via
- # ajax.
+ # Blacklist options and "Add comment" are shown if requested by
+ # an editor via ajax.
ds_diff = self.factory.makeDistroSeriesDifference()
request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
with person_logged_in(self.factory.makePerson()):
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra', request=request)
- self.assertTrue(view.show_edit_options)
-
- def test_show_edit_options_non_editor(self):
- # Even with a JS request, non-editors do not see the options.
+ self.assertTrue(view.show_add_comment)
+ self.assertFalse(view.show_blacklist_options)
+
+ def test_show_blacklist_options_for_archive_admin(self):
+ # To see the blacklist options the the user needs to be an
+ # archive admin.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ archive_admin = self.factory.makeArchiveAdmin(
+ archive=ds_diff.derived_series.main_archive)
+
+ request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
+ with person_logged_in(archive_admin):
+ view = create_initialized_view(
+ ds_diff, '+listing-distroseries-extra', request=request)
+ self.assertTrue(view.show_blacklist_options)
+
+ def test_show_add_comment_non_editor(self):
+ # Even with a JS request, non-editors do not see the 'add
+ # comment' option.
ds_diff = self.factory.makeDistroSeriesDifference()
request = LaunchpadTestRequest(HTTP_X_REQUESTED_WITH='XMLHttpRequest')
view = create_initialized_view(
ds_diff, '+listing-distroseries-extra', request=request)
- self.assertFalse(view.show_edit_options)
+ self.assertFalse(view.show_add_comment)
def test_does_display_child_diff(self):
# If the child's latest published version is not the same as the base
@@ -468,17 +485,13 @@
1, len(soup.findAll('pre', text="Here's another comment.")))
def test_blacklist_options(self):
- # blacklist options are presented to the users with
- # lp.View on the distroseries.
+ # Blacklist options are presented to the users who are archive
+ # admins.
ds_diff = self.factory.makeDistroSeriesDifference()
+ archive_admin = self.factory.makeArchiveAdmin(
+ archive=ds_diff.derived_series.main_archive)
- with person_logged_in(self.factory.makePerson()):
- self.assertTrue(
- check_permission('launchpad.Edit', ds_diff))
- self.assertTrue(
- check_permission(
- 'launchpad.View',
- ds_diff.derived_series.parent))
+ with person_logged_in(archive_admin):
request = LaunchpadTestRequest(
HTTP_X_REQUESTED_WITH='XMLHttpRequest')
view = create_initialized_view(
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2011-05-12 12:09:17 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_webservice.py 2011-05-23 17:12:36 +0000
@@ -37,10 +37,12 @@
ws_diff.self_link.endswith(ds_diff_path))
def test_blacklist(self):
- # The blacklist method can be called by people with edit access.
+ # The blacklist method can be called by people with admin access.
ds_diff = self.factory.makeDistroSeriesDifference()
+ archive_admin = self.factory.makeArchiveAdmin(
+ archive=ds_diff.derived_series.main_archive)
ws_diff = ws_object(self.factory.makeLaunchpadService(
- self.factory.makePerson()), ds_diff)
+ archive_admin), ds_diff)
result = ws_diff.blacklist()
transaction.commit()
@@ -54,11 +56,13 @@
ds_diff.status)
def test_unblacklist(self):
- # The unblacklist method can be called by people with edit access.
+ # The unblacklist method can be called by people with admin access.
ds_diff = self.factory.makeDistroSeriesDifference(
status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+ archive_admin = self.factory.makeArchiveAdmin(
+ archive=ds_diff.derived_series.main_archive)
ws_diff = ws_object(self.factory.makeLaunchpadService(
- self.factory.makePerson()), ds_diff)
+ archive_admin), ds_diff)
result = ws_diff.unblacklist()
transaction.commit()
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2011-05-14 15:02:13 +0000
+++ lib/lp/registry/configure.zcml 2011-05-23 17:12:36 +0000
@@ -162,6 +162,9 @@
class="lp.registry.model.distroseriesdifference.DistroSeriesDifference">
<allow interface="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifferencePublic"/>
<require
+ permission="launchpad.Admin"
+ interface="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifferenceAdmin"/>
+ <require
permission="launchpad.Edit"
interface="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifferenceEdit"
set_attributes="package_diff parent_package_diff"/>
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2011-05-19 16:56:14 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2011-05-23 17:12:36 +0000
@@ -8,6 +8,7 @@
__all__ = [
'IDistroSeriesDifference',
+ 'IDistroSeriesDifferenceAdmin',
'IDistroSeriesDifferencePublic',
'IDistroSeriesDifferenceEdit',
'IDistroSeriesDifferenceSource',
@@ -224,6 +225,18 @@
def addComment(commenter, comment):
"""Add a comment on this difference."""
+ @call_with(requestor=REQUEST_USER)
+ @export_write_operation()
+ def requestPackageDiffs(requestor):
+ """Requests IPackageDiffs for the derived and parent version.
+
+ :raises DistroSeriesDifferenceError: When package diffs
+ cannot be requested.
+ """
+
+class IDistroSeriesDifferenceAdmin(Interface):
+ """Difference attributes requiring launchpad.Admin."""
+
@operation_parameters(
all=Bool(title=_("All"), required=False))
@export_write_operation()
@@ -241,18 +254,10 @@
The status will be updated based on the versions.
"""
- @call_with(requestor=REQUEST_USER)
- @export_write_operation()
- def requestPackageDiffs(requestor):
- """Requests IPackageDiffs for the derived and parent version.
-
- :raises DistroSeriesDifferenceError: When package diffs
- cannot be requested.
- """
-
class IDistroSeriesDifference(IDistroSeriesDifferencePublic,
- IDistroSeriesDifferenceEdit):
+ IDistroSeriesDifferenceEdit,
+ IDistroSeriesDifferenceAdmin):
"""An interface for a package difference between two distroseries."""
export_as_webservice_entry()
=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-05-20 14:36:26 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2011-05-23 17:12:36 +0000
@@ -20,7 +20,7 @@
</metal:macro-parent>
</metal:macros>
- <tal:show_options condition="view/show_edit_options">
+ <tal:show_options condition="view/show_blacklist_options">
<div class="blacklist-options" style="float:right">
<dl>
<dt>Blacklisted:</dt>
@@ -122,7 +122,7 @@
<strong>Comments:</strong>
<tal:conversation replace="structure view/@@+render"/>
- <tal:show_options condition="view/show_edit_options"
+ <tal:show_options condition="view/show_add_comment"
define="src_name context/source_package_name/name">
<div tal:attributes="class string:add-comment-placeholder ${src_name}"></div>
</tal:show_options>
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-05-23 10:54:01 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-05-23 17:12:36 +0000
@@ -9,6 +9,7 @@
from storm.store import Store
import transaction
from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.webapp.authorization import check_permission
@@ -35,6 +36,7 @@
most_recent_publications,
)
from lp.services.propertycache import get_property_cache
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
from lp.soyuz.enums import PackageDiffStatus
from lp.soyuz.interfaces.publishing import PackagePublishingStatus
from lp.testing import (
@@ -444,11 +446,22 @@
sorted([packageset.name for packageset in packagesets]),
[packageset.name for packageset in ds_diff.packagesets])
+ def test_blacklist_unauthorised(self):
+ # If you're not an archive admin, you don't get to blacklist or
+ # unblacklist.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ random_joe = self.factory.makePerson()
+ with person_logged_in(random_joe):
+ self.assertRaises(Unauthorized, getattr, ds_diff, 'blacklist')
+ self.assertRaises(Unauthorized, getattr, ds_diff, 'unblacklist')
+
def test_blacklist_default(self):
# By default the current version is blacklisted.
ds_diff = self.factory.makeDistroSeriesDifference()
+ admin = self.factory.makeArchiveAdmin(
+ ds_diff.derived_series.main_archive)
- with person_logged_in(self.factory.makePerson()):
+ with person_logged_in(admin):
ds_diff.blacklist()
self.assertEqual(
@@ -458,8 +471,10 @@
def test_blacklist_all(self):
# All versions are blacklisted with the all=True param.
ds_diff = self.factory.makeDistroSeriesDifference()
+ admin = self.factory.makeArchiveAdmin(
+ ds_diff.derived_series.main_archive)
- with person_logged_in(self.factory.makePerson()):
+ with person_logged_in(admin):
ds_diff.blacklist(all=True)
self.assertEqual(
@@ -470,8 +485,10 @@
# Unblacklisting will return to NEEDS_ATTENTION by default.
ds_diff = self.factory.makeDistroSeriesDifference(
status=DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT)
+ admin = self.factory.makeArchiveAdmin(
+ ds_diff.derived_series.main_archive)
- with person_logged_in(self.factory.makePerson()):
+ with person_logged_in(admin):
ds_diff.unblacklist()
self.assertEqual(
@@ -492,7 +509,9 @@
status=PackagePublishingStatus.PENDING,
version='1.0')
- with person_logged_in(self.factory.makePerson()):
+ admin = self.factory.makeArchiveAdmin(
+ ds_diff.derived_series.main_archive)
+ with person_logged_in(admin):
ds_diff.unblacklist()
self.assertEqual(
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-05-20 20:40:19 +0000
+++ lib/lp/testing/factory.py 2011-05-23 17:12:36 +0000
@@ -258,6 +258,7 @@
default_name_by_purpose,
IArchiveSet,
)
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
from lp.soyuz.interfaces.component import (
@@ -2576,6 +2577,22 @@
return archive
+ def makeArchiveAdmin(self, archive=None):
+ """Make an Archive Admin.
+
+ :param archive: The `IArchive`, will be auto-created if None.
+
+ Make and return an `IPerson` who has an `ArchivePermission` to admin
+ the distroseries queue.
+ """
+ if archive is None:
+ archive = self.makeArchive()
+
+ person = self.makePerson()
+ permission_set = getUtility(IArchivePermissionSet)
+ permission_set.newQueueAdmin(archive, person, 'main')
+ return person
+
def makeBuilder(self, processor=None, url=None, name=None, title=None,
description=None, owner=None, active=True,
virtualized=True, vm_host=None, manual=False):
