Merge lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated into lp:launchpad

Proposed by Henning Eggers on 2011-04-28
Status: Merged
Merged at revision: 12943
Proposed branch: lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated
Merge into: lp:launchpad
Diff against target: 237 lines (+141/-30) 3 files modified
To merge this branch: bzr merge lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated
Reviewer Review Type Date Requested Status
Gavin Panella 2011-04-28 Approve on 2011-04-28
Review via email: mp+59364@code.launchpad.net

Commit Message

Prevent foot shooting with forwardCheckAuthenticated and add tests.

Description of the Change

= 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

To post a comment you must log in.
Gavin Panella (allenap) wrote :

This looks good, but I don't like assertVectorEqual() very much :-/
One minor gripe is that it's not much of an improvement over doing two
assertEqual() calls, but the main problem is that the error messages
won't make sense because they'll contain expected_vector and
observed_vector. In short I think it's better to simply do something
like:

        self.assertEqual(
            (1, 1),
            (adapter.checkPermissionIsRegistered.call_count,
             next_adapter.checkAuthenticated.call_count))

because then the error messages will make sense.

review: Approve
Henning Eggers (henninge) wrote :

Am 28.04.2011 15:59, schrieb Gavin Panella:
> Review: Approve

Thank you very much for your review. ;-)

>
> self.assertEqual(
> (1, 1),
> (adapter.checkPermissionIsRegistered.call_count,
> next_adapter.checkAuthenticated.call_count))
>
> because then the error messages will make sense.

Funny enough, that is exactly the code (down to the indentation) that I had
before I added assertVectorEqual. Also, it will produce the exact same output
as assertVectorEqual. I think both this solution and assertVectorEqual are
superior to two consecutive asserts because both expected/observed pairs will
be asserted whereas with the two asserts, it is not clear if the second assert
would fail, too, once the first one fails. This leads to iterative bug fixing
("Ok, we got the first one passing but now the second one if failing.")
Vector-style assertion gives a more complete picture of the situation.

The reason I introduced assertVectorEqual is that it is clear that the error
output will be a vector of observed values compared to a vector of expected
values. It's just a matter of rotating the vector from horizontal to vertical
orientation and comparing that to the code to find out which test failed.

In short: I see a far greater benefit in using assertVectorEqual that is not
outweighed by the extra work of rotating a vector in your head.

A more complex implementation could alleviate the concern about the failure
message but that is beyond the scope of this branch.

Cheers,
Henning

Preview Diff

1=== modified file 'lib/lp/app/security.py'
2--- lib/lp/app/security.py 2011-04-27 16:24:45 +0000
3+++ lib/lp/app/security.py 2011-04-28 12:48:36 +0000
4@@ -10,11 +10,9 @@
5 'AuthorizationBase',
6 ]
7
8-from zope.component import getAdapter
9+from zope.component import queryAdapter
10 from zope.interface import implements
11-from zope.security.permission import (
12- checkPermission as check_permission_is_registered,
13- )
14+from zope.security.permission import checkPermission
15
16 from canonical.launchpad.interfaces.launchpad import IPersonRoles
17 from lp.app.interfaces.security import IAuthorization
18@@ -46,6 +44,13 @@
19 """
20 return False
21
22+ def checkPermissionIsRegistered(self, obj, permission):
23+ """Pass through to checkPermission.
24+
25+ To be replaced during testing.
26+ """
27+ return checkPermission(obj, permission)
28+
29 def forwardCheckAuthenticated(self, user,
30 obj=None, permission=None):
31 """Forward request to another security adapter.
32@@ -60,15 +65,19 @@
33 permission as this adapter.
34 :return: True or False.
35 """
36+ assert obj is not None or permission is not None, (
37+ "Please specify either an object or permission to forward to.")
38 if obj is None:
39 obj = self.obj
40 if permission is None:
41 permission = self.permission
42+ # This will raise ValueError if the permission doesn't exist.
43+ self.checkPermissionIsRegistered(obj, permission)
44+ next_adapter = queryAdapter(obj, IAuthorization, permission)
45+ if next_adapter is None:
46+ return False
47 else:
48- # This will raise ValueError if the permission doesn't exist.
49- check_permission_is_registered(obj, permission)
50- next_adapter = getAdapter(obj, IAuthorization, permission)
51- return next_adapter.checkAuthenticated(user)
52+ return next_adapter.checkAuthenticated(user)
53
54 def checkAccountAuthenticated(self, account):
55 """See `IAuthorization.checkAccountAuthenticated`.
56
57=== modified file 'lib/lp/app/tests/test_security.py'
58--- lib/lp/app/tests/test_security.py 2011-04-27 16:27:25 +0000
59+++ lib/lp/app/tests/test_security.py 2011-04-28 12:48:36 +0000
60@@ -3,43 +3,135 @@
61
62 __metaclass__ = type
63
64-from canonical.testing.layers import LaunchpadFunctionalLayer
65+from zope.component import getSiteManager
66+from zope.interface import (
67+ implements,
68+ Interface,
69+ )
70+
71+from canonical.testing.layers import ZopelessDatabaseLayer
72+from lp.app.interfaces.security import IAuthorization
73 from lp.app.security import AuthorizationBase
74 from lp.testing import TestCaseWithFactory
75+from lp.testing.fakemethod import FakeMethod
76+
77+
78+class FakeSecurityAdapter(AuthorizationBase):
79+
80+ def __init__(self, adaptee=None):
81+ super(FakeSecurityAdapter, self).__init__(adaptee)
82+ self.checkAuthenticated = FakeMethod()
83+ self.checkUnauthenticated = FakeMethod()
84+
85+
86+class DummyInterface(Interface):
87+ """Marker interface to test forwarding."""
88+
89+
90+class DummyClass:
91+ """An implementation of DummyInterface."""
92+ implements(DummyInterface)
93
94
95 class TestAuthorizationBase(TestCaseWithFactory):
96
97- layer = LaunchpadFunctionalLayer
98+ layer = ZopelessDatabaseLayer
99
100- def test_default_checkAccountAuthenticated_for_full_fledged_account(self):
101+ def test_checkAccountAuthenticated_for_full_fledged_account(self):
102 # AuthorizationBase.checkAccountAuthenticated should delegate to
103 # checkAuthenticated() when the given account can be adapted into an
104 # IPerson.
105 full_fledged_account = self.factory.makePerson().account
106- adapter = TestSecurityAdapter(None)
107+ adapter = FakeSecurityAdapter()
108 adapter.checkAccountAuthenticated(full_fledged_account)
109- self.failUnless(adapter.checkAuthenticated_called)
110- self.failIf(adapter.checkUnauthenticated_called)
111+ self.assertVectorEqual(
112+ (1, adapter.checkAuthenticated.call_count),
113+ (0, adapter.checkUnauthenticated.call_count))
114
115- def test_default_checkAccountAuthenticated_for_personless_account(self):
116+ def test_checkAccountAuthenticated_for_personless_account(self):
117 # AuthorizationBase.checkAccountAuthenticated should delegate to
118 # checkUnauthenticated() when the given account can't be adapted into
119 # an IPerson.
120 personless_account = self.factory.makeAccount('Test account')
121- adapter = TestSecurityAdapter(None)
122+ adapter = FakeSecurityAdapter()
123 adapter.checkAccountAuthenticated(personless_account)
124- self.failUnless(adapter.checkUnauthenticated_called)
125- self.failIf(adapter.checkAuthenticated_called)
126-
127-
128-class TestSecurityAdapter(AuthorizationBase):
129-
130- checkAuthenticated_called = False
131- checkUnauthenticated_called = False
132-
133- def checkAuthenticated(self, user):
134- self.checkAuthenticated_called = True
135-
136- def checkUnauthenticated(self):
137- self.checkUnauthenticated_called = True
138+ self.assertVectorEqual(
139+ (0, adapter.checkAuthenticated.call_count),
140+ (1, adapter.checkUnauthenticated.call_count))
141+
142+ def _registerFakeSecurityAdpater(self, interface, permission):
143+ """Register an instance of FakeSecurityAdapter.
144+
145+ Create a factory for an instance of FakeSecurityAdapter and register
146+ it as an adapter for the given interface and permission name.
147+ """
148+ adapter = FakeSecurityAdapter()
149+
150+ def adapter_factory(adaptee):
151+ return adapter
152+
153+ getSiteManager().registerAdapter(
154+ adapter_factory, (interface,), IAuthorization, permission)
155+ return adapter
156+
157+ def test_forwardCheckAuthenticated_object_changes(self):
158+ # Requesting a check for the same permission on a different object.
159+ permission = self.factory.getUniqueString()
160+ next_adapter = self._registerFakeSecurityAdpater(
161+ DummyInterface, permission)
162+
163+ adapter = FakeSecurityAdapter()
164+ adapter.permission = permission
165+ adapter.usedfor = None
166+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
167+
168+ adapter.forwardCheckAuthenticated(None, DummyClass())
169+
170+ self.assertVectorEqual(
171+ (1, adapter.checkPermissionIsRegistered.call_count),
172+ (1, next_adapter.checkAuthenticated.call_count))
173+
174+ def test_forwardCheckAuthenticated_permission_changes(self):
175+ # Requesting a check for a different permission on the same object.
176+ next_permission = self.factory.getUniqueString()
177+ next_adapter = self._registerFakeSecurityAdpater(
178+ DummyInterface, next_permission)
179+
180+ adapter = FakeSecurityAdapter(DummyClass())
181+ adapter.permission = self.factory.getUniqueString()
182+ adapter.usedfor = DummyInterface
183+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
184+
185+ adapter.forwardCheckAuthenticated(None, permission=next_permission)
186+
187+ self.assertVectorEqual(
188+ (1, adapter.checkPermissionIsRegistered.call_count),
189+ (1, next_adapter.checkAuthenticated.call_count))
190+
191+ def test_forwardCheckAuthenticated_both_change(self):
192+ # Requesting a check for a different permission and a different
193+ # object.
194+ next_permission = self.factory.getUniqueString()
195+ next_adapter = self._registerFakeSecurityAdpater(
196+ DummyInterface, next_permission)
197+
198+ adapter = FakeSecurityAdapter()
199+ adapter.permission = self.factory.getUniqueString()
200+ adapter.usedfor = None
201+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
202+
203+ adapter.forwardCheckAuthenticated(None, DummyClass(), next_permission)
204+
205+ self.assertVectorEqual(
206+ (1, adapter.checkPermissionIsRegistered.call_count),
207+ (1, next_adapter.checkAuthenticated.call_count))
208+
209+ def test_forwardCheckAuthenticated_no_forwarder(self):
210+ # If the requested forwarding adapter does not exist, return False.
211+ adapter = FakeSecurityAdapter()
212+ adapter.permission = self.factory.getUniqueString()
213+ adapter.usedfor = DummyInterface
214+ adapter.checkPermissionIsRegistered = FakeMethod(result=True)
215+
216+ self.assertFalse(
217+ adapter.forwardCheckAuthenticated(None, DummyClass()))
218
219=== modified file 'lib/lp/testing/__init__.py'
220--- lib/lp/testing/__init__.py 2011-04-12 14:00:35 +0000
221+++ lib/lp/testing/__init__.py 2011-04-28 12:48:36 +0000
222@@ -519,6 +519,16 @@
223 lower_bound < variable < upper_bound,
224 "%r < %r < %r" % (lower_bound, variable, upper_bound))
225
226+ def assertVectorEqual(self, *args):
227+ """Apply assertEqual to all given pairs in one go.
228+
229+ Takes any number of (expected, observed) tuples and asserts each
230+ equality in one operation, thus making sure all tests are performed.
231+ If any of the tuples mismatches, AssertionError is raised.
232+ """
233+ expected_vector, observed_vector = zip(*args)
234+ return self.assertEqual(expected_vector, observed_vector)
235+
236 def pushConfig(self, section, **kwargs):
237 """Push some key-value pairs into a section of the config.
238