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

Proposed by Henning Eggers
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
lib/lp/app/security.py (+17/-8)
lib/lp/app/tests/test_security.py (+114/-22)
lib/lp/testing/__init__.py (+10/-0)
To merge this branch: bzr merge lp:~henninge/launchpad/devel-766955-fix-forwardCheckAuthenticated
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
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.
Revision history for this message
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
Revision history for this message
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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
10 'AuthorizationBase',10 'AuthorizationBase',
11 ]11 ]
1212
13from zope.component import getAdapter13from zope.component import queryAdapter
14from zope.interface import implements14from zope.interface import implements
15from zope.security.permission import (15from zope.security.permission import checkPermission
16 checkPermission as check_permission_is_registered,
17 )
1816
19from canonical.launchpad.interfaces.launchpad import IPersonRoles17from canonical.launchpad.interfaces.launchpad import IPersonRoles
20from lp.app.interfaces.security import IAuthorization18from lp.app.interfaces.security import IAuthorization
@@ -46,6 +44,13 @@
46 """44 """
47 return False45 return False
4846
47 def checkPermissionIsRegistered(self, obj, permission):
48 """Pass through to checkPermission.
49
50 To be replaced during testing.
51 """
52 return checkPermission(obj, permission)
53
49 def forwardCheckAuthenticated(self, user,54 def forwardCheckAuthenticated(self, user,
50 obj=None, permission=None):55 obj=None, permission=None):
51 """Forward request to another security adapter.56 """Forward request to another security adapter.
@@ -60,15 +65,19 @@
60 permission as this adapter.65 permission as this adapter.
61 :return: True or False.66 :return: True or False.
62 """67 """
68 assert obj is not None or permission is not None, (
69 "Please specify either an object or permission to forward to.")
63 if obj is None:70 if obj is None:
64 obj = self.obj71 obj = self.obj
65 if permission is None:72 if permission is None:
66 permission = self.permission73 permission = self.permission
74 # This will raise ValueError if the permission doesn't exist.
75 self.checkPermissionIsRegistered(obj, permission)
76 next_adapter = queryAdapter(obj, IAuthorization, permission)
77 if next_adapter is None:
78 return False
67 else:79 else:
68 # This will raise ValueError if the permission doesn't exist.80 return next_adapter.checkAuthenticated(user)
69 check_permission_is_registered(obj, permission)
70 next_adapter = getAdapter(obj, IAuthorization, permission)
71 return next_adapter.checkAuthenticated(user)
7281
73 def checkAccountAuthenticated(self, account):82 def checkAccountAuthenticated(self, account):
74 """See `IAuthorization.checkAccountAuthenticated`.83 """See `IAuthorization.checkAccountAuthenticated`.
7584
=== 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 @@
33
4__metaclass__ = type4__metaclass__ = type
55
6from canonical.testing.layers import LaunchpadFunctionalLayer6from zope.component import getSiteManager
7from zope.interface import (
8 implements,
9 Interface,
10 )
11
12from canonical.testing.layers import ZopelessDatabaseLayer
13from lp.app.interfaces.security import IAuthorization
7from lp.app.security import AuthorizationBase14from lp.app.security import AuthorizationBase
8from lp.testing import TestCaseWithFactory15from lp.testing import TestCaseWithFactory
16from lp.testing.fakemethod import FakeMethod
17
18
19class FakeSecurityAdapter(AuthorizationBase):
20
21 def __init__(self, adaptee=None):
22 super(FakeSecurityAdapter, self).__init__(adaptee)
23 self.checkAuthenticated = FakeMethod()
24 self.checkUnauthenticated = FakeMethod()
25
26
27class DummyInterface(Interface):
28 """Marker interface to test forwarding."""
29
30
31class DummyClass:
32 """An implementation of DummyInterface."""
33 implements(DummyInterface)
934
1035
11class TestAuthorizationBase(TestCaseWithFactory):36class TestAuthorizationBase(TestCaseWithFactory):
1237
13 layer = LaunchpadFunctionalLayer38 layer = ZopelessDatabaseLayer
1439
15 def test_default_checkAccountAuthenticated_for_full_fledged_account(self):40 def test_checkAccountAuthenticated_for_full_fledged_account(self):
16 # AuthorizationBase.checkAccountAuthenticated should delegate to41 # AuthorizationBase.checkAccountAuthenticated should delegate to
17 # checkAuthenticated() when the given account can be adapted into an42 # checkAuthenticated() when the given account can be adapted into an
18 # IPerson.43 # IPerson.
19 full_fledged_account = self.factory.makePerson().account44 full_fledged_account = self.factory.makePerson().account
20 adapter = TestSecurityAdapter(None)45 adapter = FakeSecurityAdapter()
21 adapter.checkAccountAuthenticated(full_fledged_account)46 adapter.checkAccountAuthenticated(full_fledged_account)
22 self.failUnless(adapter.checkAuthenticated_called)47 self.assertVectorEqual(
23 self.failIf(adapter.checkUnauthenticated_called)48 (1, adapter.checkAuthenticated.call_count),
49 (0, adapter.checkUnauthenticated.call_count))
2450
25 def test_default_checkAccountAuthenticated_for_personless_account(self):51 def test_checkAccountAuthenticated_for_personless_account(self):
26 # AuthorizationBase.checkAccountAuthenticated should delegate to52 # AuthorizationBase.checkAccountAuthenticated should delegate to
27 # checkUnauthenticated() when the given account can't be adapted into53 # checkUnauthenticated() when the given account can't be adapted into
28 # an IPerson.54 # an IPerson.
29 personless_account = self.factory.makeAccount('Test account')55 personless_account = self.factory.makeAccount('Test account')
30 adapter = TestSecurityAdapter(None)56 adapter = FakeSecurityAdapter()
31 adapter.checkAccountAuthenticated(personless_account)57 adapter.checkAccountAuthenticated(personless_account)
32 self.failUnless(adapter.checkUnauthenticated_called)58 self.assertVectorEqual(
33 self.failIf(adapter.checkAuthenticated_called)59 (0, adapter.checkAuthenticated.call_count),
3460 (1, adapter.checkUnauthenticated.call_count))
3561
36class TestSecurityAdapter(AuthorizationBase):62 def _registerFakeSecurityAdpater(self, interface, permission):
3763 """Register an instance of FakeSecurityAdapter.
38 checkAuthenticated_called = False64
39 checkUnauthenticated_called = False65 Create a factory for an instance of FakeSecurityAdapter and register
4066 it as an adapter for the given interface and permission name.
41 def checkAuthenticated(self, user):67 """
42 self.checkAuthenticated_called = True68 adapter = FakeSecurityAdapter()
4369
44 def checkUnauthenticated(self):70 def adapter_factory(adaptee):
45 self.checkUnauthenticated_called = True71 return adapter
72
73 getSiteManager().registerAdapter(
74 adapter_factory, (interface,), IAuthorization, permission)
75 return adapter
76
77 def test_forwardCheckAuthenticated_object_changes(self):
78 # Requesting a check for the same permission on a different object.
79 permission = self.factory.getUniqueString()
80 next_adapter = self._registerFakeSecurityAdpater(
81 DummyInterface, permission)
82
83 adapter = FakeSecurityAdapter()
84 adapter.permission = permission
85 adapter.usedfor = None
86 adapter.checkPermissionIsRegistered = FakeMethod(result=True)
87
88 adapter.forwardCheckAuthenticated(None, DummyClass())
89
90 self.assertVectorEqual(
91 (1, adapter.checkPermissionIsRegistered.call_count),
92 (1, next_adapter.checkAuthenticated.call_count))
93
94 def test_forwardCheckAuthenticated_permission_changes(self):
95 # Requesting a check for a different permission on the same object.
96 next_permission = self.factory.getUniqueString()
97 next_adapter = self._registerFakeSecurityAdpater(
98 DummyInterface, next_permission)
99
100 adapter = FakeSecurityAdapter(DummyClass())
101 adapter.permission = self.factory.getUniqueString()
102 adapter.usedfor = DummyInterface
103 adapter.checkPermissionIsRegistered = FakeMethod(result=True)
104
105 adapter.forwardCheckAuthenticated(None, permission=next_permission)
106
107 self.assertVectorEqual(
108 (1, adapter.checkPermissionIsRegistered.call_count),
109 (1, next_adapter.checkAuthenticated.call_count))
110
111 def test_forwardCheckAuthenticated_both_change(self):
112 # Requesting a check for a different permission and a different
113 # object.
114 next_permission = self.factory.getUniqueString()
115 next_adapter = self._registerFakeSecurityAdpater(
116 DummyInterface, next_permission)
117
118 adapter = FakeSecurityAdapter()
119 adapter.permission = self.factory.getUniqueString()
120 adapter.usedfor = None
121 adapter.checkPermissionIsRegistered = FakeMethod(result=True)
122
123 adapter.forwardCheckAuthenticated(None, DummyClass(), next_permission)
124
125 self.assertVectorEqual(
126 (1, adapter.checkPermissionIsRegistered.call_count),
127 (1, next_adapter.checkAuthenticated.call_count))
128
129 def test_forwardCheckAuthenticated_no_forwarder(self):
130 # If the requested forwarding adapter does not exist, return False.
131 adapter = FakeSecurityAdapter()
132 adapter.permission = self.factory.getUniqueString()
133 adapter.usedfor = DummyInterface
134 adapter.checkPermissionIsRegistered = FakeMethod(result=True)
135
136 self.assertFalse(
137 adapter.forwardCheckAuthenticated(None, DummyClass()))
46138
=== 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 @@
519 lower_bound < variable < upper_bound,519 lower_bound < variable < upper_bound,
520 "%r < %r < %r" % (lower_bound, variable, upper_bound))520 "%r < %r < %r" % (lower_bound, variable, upper_bound))
521521
522 def assertVectorEqual(self, *args):
523 """Apply assertEqual to all given pairs in one go.
524
525 Takes any number of (expected, observed) tuples and asserts each
526 equality in one operation, thus making sure all tests are performed.
527 If any of the tuples mismatches, AssertionError is raised.
528 """
529 expected_vector, observed_vector = zip(*args)
530 return self.assertEqual(expected_vector, observed_vector)
531
522 def pushConfig(self, section, **kwargs):532 def pushConfig(self, section, **kwargs):
523 """Push some key-value pairs into a section of the config.533 """Push some key-value pairs into a section of the config.
524534