launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04875
[Merge] lp:~bac/launchpad/sweep into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/sweep into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #839784 in Launchpad itself: "Security adapters defer to explicitly named adapters"
https://bugs.launchpad.net/launchpad/+bug/839784
For more details, see:
https://code.launchpad.net/~bac/launchpad/sweep/+merge/74112
= Summary =
In our security files, many forwarded/deferred adapters are referenced
via name, which is a means for bugs to creep in. Francis suggested a
sweep of all of our security code and replacing named lookups with
getAdapter lookups.
== Proposed fix ==
A new class ForwardedAuthorizationBase was created for classes that
forward their authorizations to child components.
== Pre-implementation notes ==
Talk with Gary.
== Implementation details ==
As above.
== Tests ==
I added one new test:
bin/test -vvm lp.app -t TestForwardedAuthorizationBase
But since so many security adapters are affected, a full run of the
test suite is the only way to ensure there were no (tested)
regressions.
== Demo and Q/A ==
N/A
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/launchpad/security.py
lib/lp/app/tests/test_security.py
lib/lp/bugs/security.py
lib/lp/app/security.py
--
https://code.launchpad.net/~bac/launchpad/sweep/+merge/74112
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/sweep into lp:launchpad.
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2011-08-31 19:45:02 +0000
+++ lib/canonical/launchpad/security.py 2011-09-05 15:17:40 +0000
@@ -37,6 +37,7 @@
from lp.app.security import (
AnonymousAuthorization,
AuthorizationBase,
+ ForwardedAuthorization,
)
from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfig
from lp.blueprints.interfaces.specification import (
@@ -1000,14 +1001,17 @@
archive.getComponentsForQueueAdmin(user.person)) or user.in_admin
-class EditDistroSeriesDifference(AuthorizationBase):
+class EditDistroSeriesDifference(ForwardedAuthorization):
"""Anyone with lp.View on the distribution can edit a DSD."""
permission = 'launchpad.Edit'
usedfor = IDistroSeriesDifferenceEdit
- def checkAuthenticated(self, user):
- return self.forwardCheckAuthenticated(
- user, self.obj.derived_series.distribution, 'launchpad.View')
+ def __init__(self, obj):
+ super(EditDistroSeriesDifference, self).__init__(
+ obj.derived_series.distribution, 'launchpad.View')
+
+ def checkUnauthenticated(self):
+ return False
class SeriesDrivers(AuthorizationBase):
@@ -1918,14 +1922,14 @@
self.forwardCheckAuthenticated(user, self.obj.distribution))
-class AdminDistributionSourcePackageTranslations(AuthorizationBase):
+class AdminDistributionSourcePackageTranslations(ForwardedAuthorization):
"""DistributionSourcePackage objects link to a distribution."""
permission = 'launchpad.TranslationsAdmin'
usedfor = IDistributionSourcePackage
- def checkAuthenticated(self, user):
- """Distribution admins are admins for source packages as well."""
- return self.forwardCheckAuthenticated(user, self.obj.distribution)
+ def __init__(self, obj):
+ super(AdminDistributionSourcePackageTranslations, self).__init__(
+ obj.distribution)
class AdminProductSeriesTranslations(AuthorizationBase):
@@ -1971,33 +1975,23 @@
self.branches))
-class PreviewDiffView(AuthorizationBase):
+class PreviewDiffView(ForwardedAuthorization):
permission = 'launchpad.View'
usedfor = IPreviewDiff
- @property
- def bmp_view(self):
- return BranchMergeProposalView(self.obj.branch_merge_proposal)
-
- def checkAuthenticated(self, user):
- """Is the user able to view the preview diff?
-
- The user can see a preview diff if they can see the merge proposal.
- """
- return self.bmp_view.checkAuthenticated(user)
-
- def checkUnauthenticated(self):
- """Is anyone able to view the branch merge proposal?
-
- The user can see a preview diff if they can see the merge proposal.
- """
- return self.bmp_view.checkUnauthenticated()
-
-
-class CodeReviewVoteReferenceEdit(AuthorizationBase):
+ def __init__(self, obj):
+ super(PreviewDiffView, self).__init__(obj.branch_merge_proposal)
+
+
+class CodeReviewVoteReferenceEdit(ForwardedAuthorization):
permission = 'launchpad.Edit'
usedfor = ICodeReviewVoteReference
+ def __init__(self, obj):
+ super(CodeReviewVoteReferenceEdit, self).__init__(
+ obj.branch_merge_proposal.target_branch)
+ self.obj = obj
+
def checkAuthenticated(self, user):
"""Only the affected teams may change the review request.
@@ -2008,57 +2002,28 @@
Anyone with edit permissions on the target branch of the merge
proposal can also edit the reviews.
"""
- if user.inTeam(self.obj.reviewer) or user.inTeam(self.obj.registrant):
- return True
- target_access = EditBranch(
- self.obj.branch_merge_proposal.target_branch)
- return target_access.checkAuthenticated(user)
-
-
-class CodeReviewCommentView(AuthorizationBase):
+ return (user.inTeam(self.obj.reviewer) or
+ user.inTeam(self.obj.registrant) or
+ super(CodeReviewVoteReferenceEdit, self).checkAuthenticated(
+ user))
+
+
+class CodeReviewCommentView(ForwardedAuthorization):
permission = 'launchpad.View'
usedfor = ICodeReviewComment
- def checkAuthenticated(self, user):
- """Is the user able to view the code review comment?
-
- The user can see a code review comment if they can see the branch
- merge proposal.
- """
- bmp_checker = BranchMergeProposalView(self.obj.branch_merge_proposal)
- return bmp_checker.checkAuthenticated(user)
-
- def checkUnauthenticated(self):
- """Are not-logged-in people able to view the code review comment?
-
- They can see a code review comment if they can see the branch merge
- proposal.
- """
- bmp_checker = BranchMergeProposalView(self.obj.branch_merge_proposal)
- return bmp_checker.checkUnauthenticated()
-
-
-class CodeReviewCommentDelete(AuthorizationBase):
+ def __init__(self, obj):
+ super(CodeReviewCommentView, self).__init__(
+ obj.branch_merge_proposal)
+
+
+class CodeReviewCommentDelete(ForwardedAuthorization):
permission = 'launchpad.Edit'
usedfor = ICodeReviewCommentDeletion
- def checkAuthenticated(self, user):
- """Is the user able to view the code review message?
-
- The user can see a code review message if they can see the branch
- merge proposal.
- """
- bmp_checker = BranchMergeProposalEdit(self.obj.branch_merge_proposal)
- return bmp_checker.checkAuthenticated(user)
-
- def checkUnauthenticated(self):
- """Are not-logged-in people able to view the code review message?
-
- They can see a code review message if they can see the branch merge
- proposal.
- """
- bmp_checker = BranchMergeProposalEdit(self.obj.branch_merge_proposal)
- return bmp_checker.checkUnauthenticated()
+ def __init__(self, obj):
+ super(CodeReviewCommentDelete, self).__init__(
+ obj.branch_merge_proposal)
class BranchMergeProposalEdit(AuthorizationBase):
@@ -2319,7 +2284,7 @@
return auth_edit.checkAuthenticated(user)
-class EditArchiveAuthToken(AuthorizationBase):
+class EditArchiveAuthToken(ForwardedAuthorization):
"""Restrict editing of archive tokens.
The user should have append privileges to the context archive, or be an
@@ -2328,14 +2293,16 @@
permission = "launchpad.Edit"
usedfor = IArchiveAuthToken
+ def __init__(self, obj):
+ super(EditArchiveAuthToken, self).__init__(
+ obj.archive, 'launchpad.Append')
+
def checkAuthenticated(self, user):
- auth_append = AppendArchive(self.obj.archive)
- if auth_append.checkAuthenticated(user):
- return True
- return user.in_admin
-
-
-class ViewPersonalArchiveSubscription(AuthorizationBase):
+ return (user.in_admin or
+ super(EditArchiveAuthToken, self).checkAuthenticated(user))
+
+
+class ViewPersonalArchiveSubscription(ForwardedAuthorization):
"""Restrict viewing of personal archive subscriptions (non-db class).
The user should be the subscriber, have append privilege to the archive
@@ -2344,18 +2311,19 @@
permission = "launchpad.View"
usedfor = IPersonalArchiveSubscription
+ def __init__(self, obj):
+ super(ViewPersonalArchiveSubscription, self).__init__(
+ obj.archive, 'launchpad.Append')
+ self.obj = obj
+
def checkAuthenticated(self, user):
- if user.person == self.obj.subscriber:
- return True
- append_archive = AppendArchive(self.obj.archive)
-
- if append_archive.checkAuthenticated(user):
- return True
-
- return user.in_admin
-
-
-class ViewArchiveSubscriber(AuthorizationBase):
+ if user.person == self.obj.subscriber or user.in_admin:
+ return True
+ return super(
+ ViewPersonalArchiveSubscription, self).checkAuthenticated(user)
+
+
+class ViewArchiveSubscriber(ForwardedAuthorization):
"""Restrict viewing of archive subscribers.
The user should be the subscriber, have append privilege to the
@@ -2364,15 +2332,17 @@
permission = "launchpad.View"
usedfor = IArchiveSubscriber
+ def __init__(self, obj):
+ super(ViewArchiveSubscriber, self).__init__(
+ obj, 'launchpad.Edit')
+ self.obj = obj
+
def checkAuthenticated(self, user):
- auth_edit = EditArchiveSubscriber(self.obj)
- result = auth_edit.checkAuthenticated(user)
- if not result:
- result = user.inTeam(self.obj.subscriber)
- return result
-
-
-class EditArchiveSubscriber(AuthorizationBase):
+ return (user.inTeam(self.obj.subscriber) or
+ super(ViewArchiveSubscriber, self).checkAuthenticated(user))
+
+
+class EditArchiveSubscriber(ForwardedAuthorization):
"""Restrict editing of archive subscribers.
The user should have append privilege to the archive or be an admin.
@@ -2380,11 +2350,13 @@
permission = "launchpad.Edit"
usedfor = IArchiveSubscriber
+ def __init__(self, obj):
+ super(EditArchiveSubscriber, self).__init__(
+ obj.archive, 'launchpad.Append')
+
def checkAuthenticated(self, user):
- auth_append = AppendArchive(self.obj.archive)
- if auth_append.checkAuthenticated(user):
- return True
- return user.in_admin
+ return (user.in_admin or
+ super(EditArchiveSubscriber, self).checkAuthenticated(user))
class DerivedAuthorization(AuthorizationBase):
@@ -2443,13 +2415,13 @@
super(ViewSourcePackagePublishingHistory, self).__init__(obj.archive)
-class EditPublishing(AuthorizationBase):
+class EditPublishing(ForwardedAuthorization):
"""Restrict editing of source and binary packages.."""
permission = "launchpad.Edit"
usedfor = IPublishingEdit
- def checkAuthenticated(self, user):
- return AppendArchive(self.obj.archive).checkAuthenticated(user)
+ def __init__(self, obj):
+ super(EditPublishing, self).__init__(obj.archive, 'launchpad.Append')
class ViewBinaryPackagePublishingHistory(ViewSourcePackagePublishingHistory):
@@ -2479,8 +2451,8 @@
def checkAuthenticated(self, user):
"""Verify that the user can view the sourcepackagerelease."""
for archive in self.obj.published_archives:
- auth_archive = ViewArchive(archive)
- if auth_archive.checkAuthenticated(user):
+ adapter = getAdapter(archive, IAuthorization, self.permission)
+ if adapter.checkAuthenticated(user):
return True
return False
=== modified file 'lib/lp/app/security.py'
--- lib/lp/app/security.py 2011-05-27 20:10:36 +0000
+++ lib/lp/app/security.py 2011-09-05 15:17:40 +0000
@@ -8,6 +8,7 @@
__all__ = [
'AnonymousAuthorization',
'AuthorizationBase',
+ 'ForwardedAuthorization',
]
from zope.component import queryAdapter
@@ -104,3 +105,31 @@
def checkAuthenticated(self, user):
"""Any authorized user can see this object."""
return True
+
+
+class ForwardedAuthorization(AuthorizationBase):
+
+ permission = None
+ usedfor = None
+
+ def __init__(self, forwarded_object, permission=None):
+ self.forwarded_object = forwarded_object
+ if permission is not None:
+ self.permission = permission
+
+ @property
+ def forwarded_adapter(self):
+ return queryAdapter(
+ self.forwarded_object, IAuthorization, self.permission)
+
+ def checkAuthenticated(self, user):
+ adapter = self.forwarded_adapter
+ if adapter is None:
+ return False
+ return adapter.checkAuthenticated(user)
+
+ def checkUnauthenticated(self):
+ adapter = self.forwarded_adapter
+ if adapter is None:
+ return False
+ return adapter.checkUnauthenticated()
=== modified file 'lib/lp/app/tests/test_security.py'
--- lib/lp/app/tests/test_security.py 2011-04-28 10:42:45 +0000
+++ lib/lp/app/tests/test_security.py 2011-09-05 15:17:40 +0000
@@ -3,7 +3,10 @@
__metaclass__ = type
-from zope.component import getSiteManager
+from zope.component import (
+ getSiteManager,
+ queryAdapter,
+ )
from zope.interface import (
implements,
Interface,
@@ -11,11 +14,31 @@
from canonical.testing.layers import ZopelessDatabaseLayer
from lp.app.interfaces.security import IAuthorization
-from lp.app.security import AuthorizationBase
+from lp.app.security import (
+ AuthorizationBase,
+ ForwardedAuthorization,
+ )
from lp.testing import TestCaseWithFactory
from lp.testing.fakemethod import FakeMethod
+def registerFakeSecurityAdapter(interface, permission, adapter=None):
+ """Register an instance of FakeSecurityAdapter.
+
+ Create a factory for an instance of FakeSecurityAdapter and register
+ it as an adapter for the given interface and permission name.
+ """
+ if adapter is None:
+ adapter = FakeSecurityAdapter()
+
+ def adapter_factory(adaptee):
+ return adapter
+
+ getSiteManager().registerAdapter(
+ adapter_factory, (interface,), IAuthorization, permission)
+ return adapter
+
+
class FakeSecurityAdapter(AuthorizationBase):
def __init__(self, adaptee=None):
@@ -24,13 +47,13 @@
self.checkUnauthenticated = FakeMethod()
-class DummyInterface(Interface):
+class IDummy(Interface):
"""Marker interface to test forwarding."""
-class DummyClass:
- """An implementation of DummyInterface."""
- implements(DummyInterface)
+class Dummy:
+ """An implementation of IDummy."""
+ implements(IDummy)
class TestAuthorizationBase(TestCaseWithFactory):
@@ -59,33 +82,18 @@
(0, adapter.checkAuthenticated.call_count),
(1, adapter.checkUnauthenticated.call_count))
- def _registerFakeSecurityAdpater(self, interface, permission):
- """Register an instance of FakeSecurityAdapter.
-
- Create a factory for an instance of FakeSecurityAdapter and register
- it as an adapter for the given interface and permission name.
- """
- adapter = FakeSecurityAdapter()
-
- def adapter_factory(adaptee):
- return adapter
-
- getSiteManager().registerAdapter(
- adapter_factory, (interface,), IAuthorization, permission)
- return adapter
-
def test_forwardCheckAuthenticated_object_changes(self):
# Requesting a check for the same permission on a different object.
permission = self.factory.getUniqueString()
- next_adapter = self._registerFakeSecurityAdpater(
- DummyInterface, permission)
+ next_adapter = registerFakeSecurityAdapter(
+ IDummy, permission)
adapter = FakeSecurityAdapter()
adapter.permission = permission
adapter.usedfor = None
adapter.checkPermissionIsRegistered = FakeMethod(result=True)
- adapter.forwardCheckAuthenticated(None, DummyClass())
+ adapter.forwardCheckAuthenticated(None, Dummy())
self.assertVectorEqual(
(1, adapter.checkPermissionIsRegistered.call_count),
@@ -94,12 +102,12 @@
def test_forwardCheckAuthenticated_permission_changes(self):
# Requesting a check for a different permission on the same object.
next_permission = self.factory.getUniqueString()
- next_adapter = self._registerFakeSecurityAdpater(
- DummyInterface, next_permission)
+ next_adapter = registerFakeSecurityAdapter(
+ IDummy, next_permission)
- adapter = FakeSecurityAdapter(DummyClass())
+ adapter = FakeSecurityAdapter(Dummy())
adapter.permission = self.factory.getUniqueString()
- adapter.usedfor = DummyInterface
+ adapter.usedfor = IDummy
adapter.checkPermissionIsRegistered = FakeMethod(result=True)
adapter.forwardCheckAuthenticated(None, permission=next_permission)
@@ -112,15 +120,15 @@
# Requesting a check for a different permission and a different
# object.
next_permission = self.factory.getUniqueString()
- next_adapter = self._registerFakeSecurityAdpater(
- DummyInterface, next_permission)
+ next_adapter = registerFakeSecurityAdapter(
+ IDummy, next_permission)
adapter = FakeSecurityAdapter()
adapter.permission = self.factory.getUniqueString()
adapter.usedfor = None
adapter.checkPermissionIsRegistered = FakeMethod(result=True)
- adapter.forwardCheckAuthenticated(None, DummyClass(), next_permission)
+ adapter.forwardCheckAuthenticated(None, Dummy(), next_permission)
self.assertVectorEqual(
(1, adapter.checkPermissionIsRegistered.call_count),
@@ -130,8 +138,62 @@
# If the requested forwarding adapter does not exist, return False.
adapter = FakeSecurityAdapter()
adapter.permission = self.factory.getUniqueString()
- adapter.usedfor = DummyInterface
+ adapter.usedfor = IDummy
adapter.checkPermissionIsRegistered = FakeMethod(result=True)
self.assertFalse(
- adapter.forwardCheckAuthenticated(None, DummyClass()))
+ adapter.forwardCheckAuthenticated(None, Dummy()))
+
+
+class FakeForwardedAuthorization(ForwardedAuthorization):
+ def __init__(self, obj, permission=None):
+ super(FakeForwardedAuthorization, self).__init__(
+ obj.child_obj, permission)
+
+
+class FakeForwardedObject:
+ implements(IDummy)
+ def __init__(self):
+ self.child_obj = Dummy()
+
+
+class TestForwardedAuthorizationBase(TestCaseWithFactory):
+
+ layer = ZopelessDatabaseLayer
+
+ def test_ForwardedAuthorization_same_permissions(self):
+
+ permission = self.factory.getUniqueString()
+ fake_obj = FakeForwardedObject()
+ outer_adapter = FakeForwardedAuthorization(fake_obj)
+ outer_adapter.permission = permission
+
+ inner_adapter = FakeSecurityAdapter()
+ inner_adapter.permission = permission
+ registerFakeSecurityAdapter(IDummy, permission, inner_adapter)
+ user = object()
+ outer_adapter.checkAuthenticated(user)
+ outer_adapter.checkUnauthenticated()
+ self.assertVectorEqual(
+ (1, inner_adapter.checkAuthenticated.call_count),
+ (1, inner_adapter.checkUnauthenticated.call_count))
+
+ def test_ForwardedAuthorization_different_permissions(self):
+ perm_inner = 'inner'
+ perm_outer = 'outer'
+ fake_obj = FakeForwardedObject()
+ outer_adapter = FakeForwardedAuthorization(fake_obj, perm_inner)
+ registerFakeSecurityAdapter(IDummy, perm_outer, outer_adapter)
+
+ inner_adapter = FakeSecurityAdapter()
+ inner_adapter.permission = perm_inner
+ registerFakeSecurityAdapter(IDummy, perm_inner, inner_adapter)
+
+ user = object()
+ adapter = queryAdapter(
+ FakeForwardedObject(), IAuthorization, perm_outer)
+ adapter.checkAuthenticated(user)
+ adapter.checkUnauthenticated()
+ self.assertVectorEqual(
+ (1, inner_adapter.checkAuthenticated.call_count),
+ (1, inner_adapter.checkUnauthenticated.call_count))
=== modified file 'lib/lp/bugs/security.py'
--- lib/lp/bugs/security.py 2011-05-12 21:33:10 +0000
+++ lib/lp/bugs/security.py 2011-09-05 15:17:40 +0000
@@ -11,11 +11,11 @@
from lp.app.security import (
AnonymousAuthorization,
AuthorizationBase,
+ ForwardedAuthorization,
)
from lp.bugs.interfaces.bug import IBug
from lp.bugs.interfaces.bugattachment import IBugAttachment
from lp.bugs.interfaces.bugbranch import IBugBranch
-from lp.bugs.interfaces.bugmessage import IBugMessage
from lp.bugs.interfaces.bugnomination import IBugNomination
from lp.bugs.interfaces.bugsubscription import IBugSubscription
from lp.bugs.interfaces.bugsubscriptionfilter import IBugSubscriptionFilter
@@ -98,11 +98,10 @@
def __init__(self, bug_branch):
# The same permissions as for the BugBranch's bug should apply
# to the BugBranch itself.
- EditPublicByLoggedInUserAndPrivateByExplicitSubscribers.__init__(
- self, bug_branch.bug)
-
-
-class ViewBugAttachment(PublicToAllOrPrivateToExplicitSubscribersForBug):
+ super(EditBugBranch, self).__init__(bug_branch.bug)
+
+
+class ViewBugAttachment(ForwardedAuthorization):
"""Security adapter for viewing a bug attachment.
If the user is authorized to view the bug, he's allowed to view the
@@ -112,12 +111,10 @@
usedfor = IBugAttachment
def __init__(self, bugattachment):
- PublicToAllOrPrivateToExplicitSubscribersForBug.__init__(
- self, bugattachment.bug)
-
-
-class EditBugAttachment(
- EditPublicByLoggedInUserAndPrivateByExplicitSubscribers):
+ super(ViewBugAttachment, self).__init__(bugattachment.bug)
+
+
+class EditBugAttachment(ForwardedAuthorization):
"""Security adapter for editing a bug attachment.
If the user is authorized to view the bug, he's allowed to edit the
@@ -127,8 +124,7 @@
usedfor = IBugAttachment
def __init__(self, bugattachment):
- EditPublicByLoggedInUserAndPrivateByExplicitSubscribers.__init__(
- self, bugattachment.bug)
+ super(EditBugAttachment, self).__init__(bugattachment.bug)
class ViewBugSubscription(AnonymousAuthorization):
