launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03472
lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated into lp:launchpad
Henning Eggers has proposed merging lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #766955 in Launchpad itself: "forwardCheckAuthenticated doesn't check existence if a permission is not specified"
https://bugs.launchpad.net/launchpad/+bug/766955
For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated/+merge/59364
= Summary =
To fix the bug is really just a two-line fix. Quite straight forward.
The new method forwardCheckAuthenticated was also lacking tests and a
defined behavior. This branch also adds that but it also adds some
complexity.
== Proposed fix ==
In lp.app.security:
- Introduce checkPermissionIsRegistered to facilitate its replacement
with FakeMethod during testing.
- Call checkPermisionIsRegistered on all code paths.
- Replace getAdapter with queryAdapter and return False when the
adapter is not found.
- Add an assertion to avoid loops.
In lp.add.tests.test_security
- Update the whole test to use FakeMethod and ZopelessDataseLayer.
- Add tests for forwardCheckAuthenticated. This is a bit complex
because it involves registering an adapter.
In lp.testing
- Introduce assertVectorEqual to test multiple values at the same time.
This formalizes a pattern that I like to use by making sure all values
are asserted. It could probably be converted to use matchers and even
be added to testtools but this implementation was the quickest for me.
(Go and read what zip() does ... ;-)
== Pre-implementation notes ==
I had talked about the fix with William after he had actually been
bitten by the bug.
== Implementation details ==
I thought I could use directlyProvides(object(), Interface) but it
seems not, so I had to introduce the dummy implementation of the
interface.
== Tests ==
bin/test -vvcm lp.app.tests.test_security
== Demo and Q/A ==
No QA possible.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/testing/__init__.py
lib/lp/app/tests/test_security.py
lib/lp/app/security.py
--
https://code.launchpad.net/~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated/+merge/59364
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated into lp:launchpad.
=== modified file 'lib/lp/app/security.py'
--- lib/lp/app/security.py 2011-04-27 16:24:45 +0000
+++ lib/lp/app/security.py 2011-04-28 12:48:36 +0000
@@ -10,11 +10,9 @@
'AuthorizationBase',
]
-from zope.component import getAdapter
+from zope.component import queryAdapter
from zope.interface import implements
-from zope.security.permission import (
- checkPermission as check_permission_is_registered,
- )
+from zope.security.permission import checkPermission
from canonical.launchpad.interfaces.launchpad import IPersonRoles
from lp.app.interfaces.security import IAuthorization
@@ -46,6 +44,13 @@
"""
return False
+ def checkPermissionIsRegistered(self, obj, permission):
+ """Pass through to checkPermission.
+
+ To be replaced during testing.
+ """
+ return checkPermission(obj, permission)
+
def forwardCheckAuthenticated(self, user,
obj=None, permission=None):
"""Forward request to another security adapter.
@@ -60,15 +65,19 @@
permission as this adapter.
:return: True or False.
"""
+ assert obj is not None or permission is not None, (
+ "Please specify either an object or permission to forward to.")
if obj is None:
obj = self.obj
if permission is None:
permission = self.permission
+ # This will raise ValueError if the permission doesn't exist.
+ self.checkPermissionIsRegistered(obj, permission)
+ next_adapter = queryAdapter(obj, IAuthorization, permission)
+ if next_adapter is None:
+ return False
else:
- # This will raise ValueError if the permission doesn't exist.
- check_permission_is_registered(obj, permission)
- next_adapter = getAdapter(obj, IAuthorization, permission)
- return next_adapter.checkAuthenticated(user)
+ return next_adapter.checkAuthenticated(user)
def checkAccountAuthenticated(self, account):
"""See `IAuthorization.checkAccountAuthenticated`.
=== modified file 'lib/lp/app/tests/test_security.py'
--- lib/lp/app/tests/test_security.py 2011-04-27 16:27:25 +0000
+++ lib/lp/app/tests/test_security.py 2011-04-28 12:48:36 +0000
@@ -3,43 +3,135 @@
__metaclass__ = type
-from canonical.testing.layers import LaunchpadFunctionalLayer
+from zope.component import getSiteManager
+from zope.interface import (
+ implements,
+ Interface,
+ )
+
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.app.interfaces.security import IAuthorization
from lp.app.security import AuthorizationBase
from lp.testing import TestCaseWithFactory
+from lp.testing.fakemethod import FakeMethod
+
+
+class FakeSecurityAdapter(AuthorizationBase):
+
+ def __init__(self, adaptee=None):
+ super(FakeSecurityAdapter, self).__init__(adaptee)
+ self.checkAuthenticated = FakeMethod()
+ self.checkUnauthenticated = FakeMethod()
+
+
+class DummyInterface(Interface):
+ """Marker interface to test forwarding."""
+
+
+class DummyClass:
+ """An implementation of DummyInterface."""
+ implements(DummyInterface)
class TestAuthorizationBase(TestCaseWithFactory):
- layer = LaunchpadFunctionalLayer
+ layer = ZopelessDatabaseLayer
- def test_default_checkAccountAuthenticated_for_full_fledged_account(self):
+ def test_checkAccountAuthenticated_for_full_fledged_account(self):
# AuthorizationBase.checkAccountAuthenticated should delegate to
# checkAuthenticated() when the given account can be adapted into an
# IPerson.
full_fledged_account = self.factory.makePerson().account
- adapter = TestSecurityAdapter(None)
+ adapter = FakeSecurityAdapter()
adapter.checkAccountAuthenticated(full_fledged_account)
- self.failUnless(adapter.checkAuthenticated_called)
- self.failIf(adapter.checkUnauthenticated_called)
+ self.assertVectorEqual(
+ (1, adapter.checkAuthenticated.call_count),
+ (0, adapter.checkUnauthenticated.call_count))
- def test_default_checkAccountAuthenticated_for_personless_account(self):
+ def test_checkAccountAuthenticated_for_personless_account(self):
# AuthorizationBase.checkAccountAuthenticated should delegate to
# checkUnauthenticated() when the given account can't be adapted into
# an IPerson.
personless_account = self.factory.makeAccount('Test account')
- adapter = TestSecurityAdapter(None)
+ adapter = FakeSecurityAdapter()
adapter.checkAccountAuthenticated(personless_account)
- self.failUnless(adapter.checkUnauthenticated_called)
- self.failIf(adapter.checkAuthenticated_called)
-
-
-class TestSecurityAdapter(AuthorizationBase):
-
- checkAuthenticated_called = False
- checkUnauthenticated_called = False
-
- def checkAuthenticated(self, user):
- self.checkAuthenticated_called = True
-
- def checkUnauthenticated(self):
- self.checkUnauthenticated_called = True
+ self.assertVectorEqual(
+ (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)
+
+ adapter = FakeSecurityAdapter()
+ adapter.permission = permission
+ adapter.usedfor = None
+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
+
+ adapter.forwardCheckAuthenticated(None, DummyClass())
+
+ self.assertVectorEqual(
+ (1, adapter.checkPermissionIsRegistered.call_count),
+ (1, next_adapter.checkAuthenticated.call_count))
+
+ 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)
+
+ adapter = FakeSecurityAdapter(DummyClass())
+ adapter.permission = self.factory.getUniqueString()
+ adapter.usedfor = DummyInterface
+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
+
+ adapter.forwardCheckAuthenticated(None, permission=next_permission)
+
+ self.assertVectorEqual(
+ (1, adapter.checkPermissionIsRegistered.call_count),
+ (1, next_adapter.checkAuthenticated.call_count))
+
+ def test_forwardCheckAuthenticated_both_change(self):
+ # Requesting a check for a different permission and a different
+ # object.
+ next_permission = self.factory.getUniqueString()
+ next_adapter = self._registerFakeSecurityAdpater(
+ DummyInterface, next_permission)
+
+ adapter = FakeSecurityAdapter()
+ adapter.permission = self.factory.getUniqueString()
+ adapter.usedfor = None
+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
+
+ adapter.forwardCheckAuthenticated(None, DummyClass(), next_permission)
+
+ self.assertVectorEqual(
+ (1, adapter.checkPermissionIsRegistered.call_count),
+ (1, next_adapter.checkAuthenticated.call_count))
+
+ def test_forwardCheckAuthenticated_no_forwarder(self):
+ # If the requested forwarding adapter does not exist, return False.
+ adapter = FakeSecurityAdapter()
+ adapter.permission = self.factory.getUniqueString()
+ adapter.usedfor = DummyInterface
+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
+
+ self.assertFalse(
+ adapter.forwardCheckAuthenticated(None, DummyClass()))
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2011-04-12 14:00:35 +0000
+++ lib/lp/testing/__init__.py 2011-04-28 12:48:36 +0000
@@ -519,6 +519,16 @@
lower_bound < variable < upper_bound,
"%r < %r < %r" % (lower_bound, variable, upper_bound))
+ def assertVectorEqual(self, *args):
+ """Apply assertEqual to all given pairs in one go.
+
+ Takes any number of (expected, observed) tuples and asserts each
+ equality in one operation, thus making sure all tests are performed.
+ If any of the tuples mismatches, AssertionError is raised.
+ """
+ expected_vector, observed_vector = zip(*args)
+ return self.assertEqual(expected_vector, observed_vector)
+
def pushConfig(self, section, **kwargs):
"""Push some key-value pairs into a section of the config.
