Merge lp:~canonical-isd-hackers/canonical-identity-provider/captcha-tangle into lp:canonical-identity-provider/release

[David Owen]

Pull a dependency on request objects out of the Captcha model layer.

The Captcha model code had a dependency on fields in the WSGI request object, making the code very difficult to change (for example, to change our API to Piston or similar).

This patch pulls that dependency much close to the view layer.

The web API itself should have remain unchanged.

=== modified file 'identityprovider/models/captcha.py'
--- identityprovider/models/captcha.py	2010-09-24 13:17:26 +0000
+++ identityprovider/models/captcha.py	2011-01-25 22:24:43 +0000
@@ -11,6 +11,25 @@
 from django.conf import settings
 
 
+class CaptchaError(Exception):
+    """All instances of CaptchaError have a traceback member."""
+
+    def __init__(self, traceback):
+        self.traceback = traceback
+
+
+class NewCaptchaError(CaptchaError):
+    """The dummy member is a blank Captcha response."""
+
+    def __init__(self, traceback, dummy):
+        super(NewCaptchaError, self).__init__(traceback)
+        self.dummy = dummy
+
+
+class VerifyCaptchaError(CaptchaError):
+    pass
+
+
 class CaptchaResponse(object):
     """Response returned from _open() in case of error"""
 
@@ -49,13 +68,13 @@
     As is verifying received solution:
 
         >>> captcha = Captcha('captcha-id-received-from-client')
-        >>> captcha.verify("this-is-invalid-solution")
+        >>> captcha.verify("this-is-invalid-solution", ip_addr)
         False
 
     Once verified solution is cached, so calling again to .verify() method is
     very cheap (and returns same result):
 
-        >>> captcha.verify("this-is-invalid-solution")
+        >>> captcha.verify("this-is-invalid-solution", ip_addr)
         False
 
     You can also get original response from reCaptcha:
@@ -68,8 +87,10 @@
 
     opener = None
 
-    def __init__(self, env, captcha_id, image_url=None, response=None):
-        self.env = env
+    def __init__(self, captcha_id, image_url=None, response=None):
+        assert captcha_id is None or isinstance(captcha_id, basestring)
+        assert image_url is None or isinstance(image_url, basestring)
+        assert response is None or isinstance(response, CaptchaResponse)
         self.captcha_id = captcha_id
         self.image_url = image_url
         self.response = response
@@ -96,16 +117,13 @@
         }
 
     @classmethod
-    def new(cls, env):
+    def new(cls):
         cls._setup_opener()
         url = (settings.CAPTCHA_API_URL +
                '/challenge?k=%s' % settings.CAPTCHA_PUBLIC_KEY)
-        response = cls._open(url, env)
+        response = cls._open(url)
         if response.is_error:
-            env['oops-dump'] = True
-            logging.warning(response.traceback)
-            logging.warning("Failed to connect to reCaptcha server")
-            return cls(env, None, None)
+            raise NewCaptchaError(response.traceback, cls(None, None))
 
         data = response.data()
         m = re.search(r"challenge\s*:\s*'(.+?)'", data, re.M | re.S)
@@ -114,10 +132,10 @@
             image_url = settings.CAPTCHA_IMAGE_URL_PATTERN % captcha_id
         else:
             captcha_id, image_url = None, None
-        return cls(env, captcha_id, image_url, response)
+        return cls(captcha_id, image_url, response)
 
     @classmethod
-    def _open(cls, request, env):
+    def _open(cls, request):
         default_timeout = socket.getdefaulttimeout()
         socket.setdefaulttimeout(getattr(settings, 'CAPTCHA_TIMEOUT', 10))
 
@@ -142,7 +160,7 @@
 
         return CaptchaResponse(response.code, response, None)
 
-    def verify(self, captcha_solution):
+    def verify(self, captcha_solution, remote_ip):
         if self._verified is not None:
             return self._verified
 
@@ -155,12 +173,12 @@
 
         request_data = urllib.urlencode({
             'privatekey': settings.CAPTCHA_PRIVATE_KEY,
-            'remoteip': self.env['REMOTE_ADDR'],
+            'remoteip': remote_ip,
             'challenge': self.captcha_id,
             'response': captcha_solution,
         })
         request = urllib2.Request(settings.CAPTCHA_VERIFY_URL, request_data)
-        self.response = self._open(request, self.env)
+        self.response = self._open(request)
 
         if not self.response.is_error:
             response_data = self.response.data()
@@ -171,8 +189,5 @@
             self._verfied = False
         else:
             self._verified = False
-            if getattr(self.env, '__setitem__', False):
-                self.env['oops-dump'] = True
-            logging.warning(self.response.traceback)
-            logging.warning("reCaptcha connection error")
+            raise VerifyCaptchaError(self.response.traceback)
         return self._verified

=== modified file 'identityprovider/tests/test_captcha.py'
--- identityprovider/tests/test_captcha.py	2010-11-03 16:06:08 +0000
+++ identityprovider/tests/test_captcha.py	2011-01-25 22:24:43 +0000
@@ -26,7 +26,7 @@
     def test_opener_is_created_during_init(self):
         Captcha.opener = None
 
-        captcha = Captcha({}, 'test')
+        captcha = Captcha('test')
 
         self.assertTrue(Captcha.opener is not None)
 
@@ -41,10 +41,10 @@
             encoded_solution = captcha_solution.encode('utf-8')
             mock_urlencode.return_value = 'some text'
 
-            captcha = Captcha({'REMOTE_ADDR': 'foobar'}, None)
+            captcha = Captcha(None)
             captcha._open = Mock()
             captcha._open.return_value.is_error = True
-            captcha.verify(captcha_solution)
+            captcha.verify(captcha_solution, 'foobar')
 
             captcha_response = mock_urlencode.call_args[0][0]['response']
             self.assertEqual(captcha_response, encoded_solution)
@@ -56,7 +56,7 @@
         settings.CAPTCHA_USE_PROXY = True
         settings.CAPTCHA_PROXIES = {}
 
-        captcha = Captcha({}, 'test')
+        captcha = Captcha('test')
 
         self.assertTrue(Captcha.opener is not None)
 
@@ -73,20 +73,20 @@
         Captcha.opener.open = mock_open
 
         while handled_codes:
-            response = Captcha._open(None, {})
+            response = Captcha._open(None)
             self.assertTrue(response.is_error)
 
     def test_verify_is_short_circuited_when_disabled_in_settings(self):
         settings.DISABLE_CAPTCHA_VERIFICATION = True
 
-        captcha = Captcha({'REMOTE_ADDR': '127.0.0.1'}, 'test')
-        verify_result = captcha.verify('solution')
+        captcha = Captcha('test')
+        verify_result = captcha.verify('solution', '127.0.0.1')
 
         self.assertTrue(verify_result)
         self.assertTrue(captcha.response is None)
 
     def test_serialize(self):
-        captcha = Captcha({}, 'id', 'image')
+        captcha = Captcha('id', 'image')
 
         self.assertEquals(captcha.serialize(),
                           {'captcha_id': 'id', 'image_url': 'image'})

=== modified file 'identityprovider/tests/test_forms.py'
--- identityprovider/tests/test_forms.py	2010-07-05 16:15:30 +0000
+++ identityprovider/tests/test_forms.py	2011-01-25 22:24:43 +0000
@@ -84,7 +84,8 @@
         forms.get_current_browser_request = self.old_get_request
 
     def test_nonascii_password(self):
-        data = {'password': 'Curuzú Cuatiá'}
+        data = {'password': 'Curuzú Cuatiá',
+                'remote_ip': '127.0.0.1'}
         form = WebserviceCreateAccountForm(data=data)
         self.assertFalse(form.is_valid())
         self.assertEqual(form.errors['password'][0],

=== modified file 'identityprovider/tests/test_models_captcha.py'
--- identityprovider/tests/test_models_captcha.py	2010-10-25 20:17:15 +0000
+++ identityprovider/tests/test_models_captcha.py	2011-01-25 22:24:43 +0000
@@ -9,7 +9,12 @@
 from cStringIO import StringIO
 from django.conf import settings
 
-from identityprovider.models.captcha import Captcha, CaptchaResponse
+from identityprovider.models.captcha import (
+    Captcha,
+    CaptchaResponse,
+    NewCaptchaError,
+    VerifyCaptchaError,
+)
 
 
 class CaptchaResponseTestCase(unittest.TestCase):
@@ -33,15 +38,16 @@
 class CaptchaTestCase(unittest.TestCase):
 
     @patch_object(Captcha, '_open')
-    def test_new_captcha_oops(self, mock_open):
-        c = Captcha.new({})
-        self.assertTrue(c.env['oops-dump'])
+    def test_new_captcha_error(self, mock_open):
+        self.assertRaises(NewCaptchaError, Captcha.new)
 
     @patch_object(Captcha, '_open')
     def test_new_captcha_no_challenge(self, mock_open):
-        c = Captcha.new({})
-        self.assertEqual(c.captcha_id, None)
-        self.assertEqual(c.image_url, None)
+        try:
+            Captcha.new()
+        except NewCaptchaError, e:
+            self.assertEqual(e.dummy.captcha_id, None)
+            self.assertEqual(e.dummy.image_url, None)
 
     def test_captcha_open(self):
         class MockOpener(object):
@@ -50,7 +56,7 @@
         old_opener = Captcha.opener
         Captcha.opener = MockOpener()
 
-        self.assertRaises(urllib2.URLError, Captcha.new, {})
+        self.assertRaises(urllib2.URLError, Captcha.new)
 
         Captcha.opener = old_opener
 
@@ -66,30 +72,30 @@
 
     @patch_object(Captcha, '_open')
     def test_verify_already_verified(self, mock_open):
-        c = Captcha.new({})
+        c = Captcha(None)
         c._verified = True
-        r = c.verify(None)
+        r = c.verify(None, None)
         self.assertTrue(r)
 
     @patch_object(Captcha, '_open')
     def test_verify_no_verification(self, mock_open):
         settings.DISABLE_CAPTCHA_VERIFICATION = True
 
-        c = Captcha.new({})
-        r = c.verify(None)
+        c = Captcha(None)
+        r = c.verify(None, None)
         self.assertTrue(r)
 
     @patch_object(Captcha, '_open')
     def test_verify_response_ok(self, mock_open):
         @classmethod
-        def mock_open(cls, request, env):
+        def mock_open(cls, request):
             r = CaptchaResponse(200, StringIO('true\nok'))
             return r
         old_open = Captcha._open
         Captcha._open = mock_open
 
-        c = Captcha.new({'REMOTE_ADDR': 'localhost'})
-        r = c.verify(None)
+        c = Captcha.new()
+        r = c.verify(None, "localhost")
 
         self.assertTrue(r)
         self.assertEqual(c.message, 'ok')
@@ -98,34 +104,15 @@
 
     @patch_object(Captcha, '_open')
     def test_verify_no_captcha_id(self, mock_open):
-        c = Captcha.new({'REMOTE_ADDR': 'localhost'})
-        r = c.verify(None)
+        c = Captcha(None)
+        r = c.verify(None, 'localhost')
 
         self.assertFalse(r)
         self.assertEqual(c.message, 'no-challenge')
 
     @patch_object(Captcha, '_open')
     def test_verify_error(self, mock_open):
-        class MockEnv(dict):
-            def __getattribute__(self, attr):
-                if attr == '__setitem__':
-                    raise AttributeError()
-                else:
-                    return super(MockEnv, self).__getattribute__(attr)
-
-        env = MockEnv({'REMOTE_ADDR': 'localhost'})
-        c = Captcha(env, 'challenge-id')
-        r = c.verify(None)
-
-        self.assertFalse(r)
-        self.assertFalse('oops-dump' in c.env)
-
-    @patch_object(Captcha, '_open')
-    def test_verify_error_oops(self, mock_open):
-        c = Captcha.new({'REMOTE_ADDR': 'localhost'})
+        c = Captcha(None)
         c.captcha_id = 'challenge-id'
-        r = c.verify(None)
-
-        self.assertFalse(r)
-        self.assertTrue(c.env['oops-dump'])
-
+
+        self.assertRaises(VerifyCaptchaError, c.verify, None, 'localhost')

=== modified file 'identityprovider/tests/test_views_ui.py'
--- identityprovider/tests/test_views_ui.py	2010-11-11 13:07:16 +0000
+++ identityprovider/tests/test_views_ui.py	2011-01-25 22:24:43 +0000
@@ -565,7 +565,7 @@
             def __init__(self, *args):
                 pass
 
-            def verify(self, solution):
+            def verify(self, solution, ip_addr):
                 self.message = 'no-challenge'
                 return False
 

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2010-11-16 19:36:42 +0000
+++ identityprovider/views/ui.py	2011-01-25 22:24:43 +0000
@@ -29,7 +29,7 @@
     TokenForm)
 from identityprovider.models import (Account, AccountPassword, AuthToken,
     AuthTokenFactory, EmailAddress, get_type_of_token, verify_token_string)
-from identityprovider.models.captcha import Captcha
+from identityprovider.models.captcha import Captcha, VerifyCaptchaError
 from identityprovider.models.const import (AccountStatus, EmailStatus,
     LoginTokenType)
 from identityprovider.models.openidmodels import OpenIDRPConfig
@@ -421,17 +421,26 @@
 
 
 def _verify_captcha_response(template, request, form):
-    captcha = Captcha(request.environ,
-                      request.POST.get('recaptcha_challenge_field'))
+    captcha = Captcha(request.POST.get('recaptcha_challenge_field'))
     captcha_solution = request.POST.get('recaptcha_response_field')
-    verified = captcha.verify(captcha_solution)
-    if not verified:
-        return render_to_response(template,
-                RequestContext(request, {'form': form,
-                 'CAPTCHA_PUBLIC_KEY': settings.CAPTCHA_PUBLIC_KEY,
-                 'captcha_error': ('&error=%s' % captcha.message),
-                 }))
-    return None
+    ip_addr = request.environ["REMOTE_ADDR"]
+    try:
+        verified = captcha.verify(captcha_solution, ip_addr)
+        if verified:
+            return None
+    except VerifyCaptchaError, e:
+        if getattr(request.environ, '__setitem__', False):
+            request.environ['oops-dump'] = True
+        logging.warning(e.traceback)
+        logging.warning("reCaptcha connection error")
+
+    # not verified
+    return render_to_response(
+        template,
+        RequestContext(request, {
+            'form': form,
+            'CAPTCHA_PUBLIC_KEY': settings.CAPTCHA_PUBLIC_KEY,
+            'captcha_error': ('&error=%s' % captcha.message)}))
 
 
 # {{workflow}}

=== modified file 'identityprovider/webservice/forms.py'
--- identityprovider/webservice/forms.py	2010-08-23 14:51:12 +0000
+++ identityprovider/webservice/forms.py	2011-01-25 22:24:43 +0000
@@ -9,7 +9,7 @@
 from lazr.restful.utils import get_current_browser_request
 
 from identityprovider.utils import password_policy_compliant
-from identityprovider.models.captcha import Captcha
+from identityprovider.models.captcha import Captcha, VerifyCaptchaError
 
 
 PASSWORD_POLICY_ERROR = _("Password must be at least "
@@ -22,6 +22,7 @@
     password = fields.CharField(max_length=256)
     captcha_id = fields.CharField(max_length=1024)
     captcha_solution = fields.CharField(max_length=256)
+    remote_ip = fields.CharField(max_length=256)
 
     def clean_password(self):
         if 'password' in self.cleaned_data:
@@ -40,9 +41,24 @@
         captcha_id = cleaned_data.get('captcha_id')
         captcha_solution = cleaned_data.get('captcha_solution')
 
+        # The remote IP address is absolutely required, and comes from
+        # SSO itself, not from the client.  If it's missing, it's a
+        # programming error, and should not be returned to the client
+        # as a validation error.  So, we use a normal key lookup here.
+        remote_ip = cleaned_data['remote_ip']
+
         request = get_current_browser_request()
-        captcha = Captcha(request.environment, captcha_id)
-        if not captcha.verify(captcha_solution):
-            raise forms.ValidationError(_("Wrong captcha solution."))
-
-        return cleaned_data
+
+        captcha = Captcha(captcha_id)
+
+        try:
+            if captcha.verify(captcha_solution, remote_ip):
+                return cleaned_data
+        except VerifyCaptchaError, e:
+            if getattr(request.environment, '__setitem__', False):
+                request.environment['oops-dump'] = True
+            logging.warning(e.traceback)
+            logging.warning("reCaptcha connection error")
+
+        # not verified
+        raise forms.ValidationError(_("Wrong captcha solution."))

=== modified file 'identityprovider/webservice/models.py'
--- identityprovider/webservice/models.py	2011-01-24 18:28:24 +0000
+++ identityprovider/webservice/models.py	2011-01-25 22:24:43 +0000
@@ -26,7 +26,10 @@
     IAuthenticationSet, IRegistrationSet, ICaptchaSet)
 from identityprovider.webservice.forms import (
     WebserviceCreateAccountForm, PASSWORD_POLICY_ERROR)
-from identityprovider.models.captcha import Captcha as CaptchaModel
+from identityprovider.models.captcha import (
+    Captcha as CaptchaModel,
+    NewCaptchaError,
+)
 from identityprovider.views.server import get_team_memberships
 from identityprovider.utils import (CannotResetPasswordException,
     PersonAndAccountNotFoundException, get_person_and_account_by_email)
@@ -225,6 +228,16 @@
 class RegistrationSet(make_set('BaseRegistrationSet', 'registration',
                                IRegistrationSet, Account, 'id')):
     def register(self, **kwargs):
+        request = get_current_browser_request()
+        # TODO: request doesn't exist in a test, unless we go through
+        # the test client browser.
+        if request is None:
+            kwargs['remote_ip'] = '127.0.0.1'
+        else:
+            if hasattr(request, 'environment'):
+                kwargs['remote_ip'] = request.environment['REMOTE_ADDR']
+            else:
+                kwargs['remote_ip'] = request.environ['REMOTE_ADDR']
         form = WebserviceCreateAccountForm(kwargs)
         if not form.is_valid():
             errors = dict((k, map(unicode, v))
@@ -341,7 +354,15 @@
                           Captcha, 'id')):
     def new(self):
         request = get_current_browser_request()
-        return CaptchaModel.new(request.environment).serialize()
+        try:
+            return CaptchaModel.new().serialize()
+        except NewCaptchaError, e:
+            request.environment['oops-dump'] = True
+            logging.warning(e.traceback)
+            logging.warning("Failed to connect to reCaptcha server")
+            # TODO: Some better return here?
+            return e.dummy
+
 
 
 class SSOWebServiceRootResource(RootResource):