launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/devel-764406-security-adapters into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #764406 in Launchpad itself: "check_permission is (incorrectly) use security adapters which can check the wrong users permissions"
  https://bugs.launchpad.net/launchpad/+bug/764406

For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-764406-security-adapters/+merge/58109

= Summary =

The problem is explained in the bug 764406.
The threat may be more of a theoretical nature. Nevertheless, this fix
makes sure that checkAuthenticated really does what it proclaims: it
checks permissions for the given authenticated user and not a user it
figures out on its own.

== Proposed fix ==

Add a method on Authorization base to forward checkAuthenticated requests
to another security adapter.

== Pre-implementation notes ==

Robert seems to be agreeing that this needs to be fixed. ;-)


== Implementation details ==

I copied the use of check_permission_is_registered from the
implementation of check_permission because arbitrary strings might be
passed into the the forwarding methods.

== Tests ==

I replaced check_permission in a few security adapters but to be sure
only the whole test suite will be enough. ;-)

== Demo and Q/A ==

Similar: Check that LP is still working.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
-- 
https://code.launchpad.net/~henninge/launchpad/devel-764406-security-adapters/+merge/58109
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-764406-security-adapters into lp:launchpad.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-04-14 15:26:55 +0000
+++ lib/canonical/launchpad/security.py	2011-04-18 11:20:58 +0000
@@ -19,6 +19,9 @@
     implements,
     Interface,
     )
+from zope.security.permission import (
+    checkPermission as check_permission_is_registered,
+    )
 
 from canonical.config import config
 from canonical.launchpad.interfaces.account import IAccount
@@ -36,7 +39,6 @@
     IOAuthAccessToken,
     IOAuthRequestToken,
     )
-from canonical.launchpad.webapp.authorization import check_permission
 from canonical.launchpad.webapp.interfaces import (
     IAuthorization,
     ILaunchpadRoot,
@@ -238,6 +240,30 @@
         """
         return False
 
+    def forwardCheckAuthenticated(self, user,
+                                  obj=None, permission=None):
+        """Forward request to another security adapter.
+
+        Find a matching adapter and call checkAuthenticated on it. Intended
+        to be used in checkAuthenticated.
+
+        :param user: The IRolesPerson object that was passed in.
+        :param obj: The object to check the permission for. If None, use
+            the same object as this adapter.
+        :param permission: The permission to check. If None, use the same
+            permission as this adapter.
+        :return: True or False.
+        """
+        if obj is None:
+            obj = self.obj
+        if permission is None:
+            permission = self.permission
+        else:
+            # This will raise ValueError if the permission doesn't exist.
+            check_permission_is_registered(permission, obj)
+        next_adapter = getAdapter(obj, IAuthorization, permission)
+        return next_adapter.checkAuthenticated(user)
+
     def checkAccountAuthenticated(self, account):
         """See `IAuthorization.checkAccountAuthenticated`.
 
@@ -560,7 +586,7 @@
         # extremely difficult to do :-)
         return (
             self.obj.goal and
-            check_permission("launchpad.Driver", self.obj.goal))
+            self.forwardCheckAuthenticated(user, self.obj.goal))
 
 
 class EditSprintSpecification(AuthorizationBase):
@@ -1025,8 +1051,8 @@
     usedfor = IDistroSeriesDifferenceEdit
 
     def checkAuthenticated(self, user):
-        return check_permission(
-            'launchpad.View', self.obj.derived_series.distribution)
+        return self.forwardCheckAuthenticated(
+            user, self.obj.derived_series.distribution, 'launchpad.View')
 
 
 class SeriesDrivers(AuthorizationBase):
@@ -1116,7 +1142,8 @@
 
         # Removal of a target cascades removals to StructuralSubscriptions,
         # so we need to allow editing to those who can edit the target itself.
-        can_edit_target = check_permission('launchpad.Edit', self.obj.target)
+        can_edit_target = self.forwardCheckAuthenticated(
+            user, self.obj.target)
 
         # Who is actually allowed to edit a subscription is determined by
         # a helper method on the model.
@@ -2047,7 +2074,8 @@
         """
         return (user.inTeam(self.obj.registrant) or
                 user.inTeam(self.obj.source_branch.owner) or
-                check_permission('launchpad.Edit', self.obj.target_branch) or
+                self.forwardCheckAuthenticated(
+                    user, self.obj.target_branch) or
                 user.inTeam(self.obj.target_branch.reviewer))
 
 
@@ -2588,7 +2616,7 @@
         parent = getattr(self.obj, '__parent__', None)
         if parent is None:
             return False
-        return check_permission(self.permission, parent)
+        return self.forwardCheckAuthenticated(user, parent)
 
 
 class ViewLibraryFileAliasWithParent(AuthorizationBase):
@@ -2608,7 +2636,7 @@
         parent = getattr(self.obj, '__parent__', None)
         if parent is None:
             return False
-        return check_permission(self.permission, parent)
+        return self.forwardCheckAuthenticated(user, parent)
 
 
 class SetMessageVisibility(AuthorizationBase):