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

[David Owen]

[Natalia Bidart (nataliabidart)]

Rejecting since this is pretty old.

=== modified file 'identityprovider/models/captcha.py'
--- identityprovider/models/captcha.py	2010-09-24 13:17:26 +0000
+++ identityprovider/models/captcha.py	2011-01-20 22:13:02 +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_admin.py'
=== 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-20 22:13:02 +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-20 22:13:02 +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-20 22:13:02 +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-20 22:13:02 +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-20 22:13:02 +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-20 22:13:02 +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-11 15:50:14 +0000
+++ identityprovider/webservice/models.py	2011-01-20 22:13:02 +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):

=== modified file 'scripts/test'
--- scripts/test	2010-12-16 20:59:34 +0000
+++ scripts/test	2011-01-20 22:13:02 +0000
@@ -144,6 +144,8 @@
 # Creating nice (HTML) coverage report
 rm -rf $PROJECT_DIR/coverage
 
+exit
+
 # coverage found in Lucid won't create the output directory on its
 # own.
 mkdir $PROJECT_DIR/coverage