Merge lp:~maxiberta/canonical-identity-provider/drop-recaptcha-v1 into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Drop the last bits of reCaptcha V1 code.

reCaptcha v1 is dead since March 2018. This is the final branch of reCaptcha v1 cleanup (see prerequisite branch).

[Matias Bordese (matiasb)]

LGTM

review: Approve

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/202/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/203/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/204/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/205/

=== modified file 'django_project/settings_base.py'
--- django_project/settings_base.py	2018-10-19 19:28:43 +0000
+++ django_project/settings_base.py	2018-10-25 17:56:53 +0000
@@ -58,13 +58,9 @@
 CACHE_MIDDLEWARE_ALIAS = 'default'
 CACHE_MIDDLEWARE_KEY_PREFIX = ''
 CACHE_MIDDLEWARE_SECONDS = 600
-CAPTCHA_API_URL = 'http://www.google.com/recaptcha/api'
-CAPTCHA_API_URL_SECURE = 'https://www.google.com/recaptcha/api'
-CAPTCHA_IMAGE_URL_PATTERN = 'https://www.google.com/recaptcha/api/image?c=%s'
 CAPTCHA_PRIVATE_KEY = ''
 CAPTCHA_PROXIES = {}
 CAPTCHA_PUBLIC_KEY = ''
-CAPTCHA_VERIFY_URL = 'http://www.google.com/recaptcha/api/verify'
 CAPTCHA_V2_VERIFY_URL = 'https://www.google.com/recaptcha/api/siteverify'
 COMBINE = True
 COMBO_URL = '/combo/'

=== modified file 'src/api/v10/tests/utils.py'
--- src/api/v10/tests/utils.py	2018-08-17 18:38:29 +0000
+++ src/api/v10/tests/utils.py	2018-10-25 17:56:53 +0000
@@ -43,9 +43,3 @@
         add_wsgi_intercept(parse.hostname, parse.port, WSGIHandler)
         self.addCleanup(remove_wsgi_intercept, parse.hostname, parse.port)
         self.api = ServiceRoot(None, host_url + '/api/1.0')
-
-        # patch Captcha so it never hits the real network
-        self.mock_captcha_open = self.patch(
-            'identityprovider.models.captcha.Captcha._open')
-        self.mock_captcha_open.return_value.is_error = False
-        self.mock_captcha_open.return_value.data.return_value = 'true\nyey'

=== modified file 'src/identityprovider/models/captcha.py'
--- src/identityprovider/models/captcha.py	2018-02-09 20:56:16 +0000
+++ src/identityprovider/models/captcha.py	2018-10-25 17:56:53 +0000
@@ -22,14 +22,6 @@
         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):
     def __init__(self, response):
         super(VerifyCaptchaError, self).__init__(response.traceback)
@@ -122,122 +114,6 @@
         return False
 
 
-class Captcha(CaptchaBase):
-    """Class to capture & abstract interaction with Google reCaptcha v1.
-
-    .. note: There's no possibility to actually test verification actually
-             returning true, as response, for that you need human interaction.
-
-    Getting new captcha is quite simple:
-
-        >>> captcha = Captcha.new()
-        >>> captcha.serialize() #+doctest: ELLIPSIS
-        {'captcha_id': ..., 'image_url': ...}
-
-    As is verifying received solution:
-
-        >>> ip_addr = '1.1.1.1'
-        >>> email = '[e-mail]'
-        >>> captcha = Captcha('captcha-id-received-from-client', email)
-        >>> captcha.verify("this-is-invalid-solution", ip_addr, email)
-        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", ip_addr, email)
-        False
-
-    You can also get original response from reCaptcha:
-
-        >>> print(captcha.response.data())
-        true
-        success
-
-    """
-
-    def __init__(self, captcha_id, image_url=None, response=None):
-        assert captcha_id is None or isinstance(captcha_id, basestring), (
-            'captcha_id should be either None or string: %r' % captcha_id)
-        assert image_url is None or isinstance(image_url, basestring), (
-            'image_url should be either None or string: %r' % image_url)
-        assert response is None or isinstance(response, CaptchaResponse), (
-            'response should be either None or a CaptchaResponse: %r' %
-            response)
-        self.captcha_id = captcha_id
-        self.image_url = image_url
-        self.response = response
-        self._verified = None
-        super(Captcha, self).__init__()
-
-    def serialize(self):
-        return {
-            'captcha_id': self.captcha_id,
-            'image_url': self.image_url,
-        }
-
-    @classmethod
-    def new(cls, timer=None):
-        cls._setup_opener()
-
-        if not settings.CAPTCHA_PUBLIC_KEY:
-            raise EmptyCaptchaKey(
-                'Captcha.open: CAPTCHA_PUBLIC_KEY setting is unset.')
-
-        url = (settings.CAPTCHA_API_URL +
-               '/challenge?k=%s' % settings.CAPTCHA_PUBLIC_KEY)
-        with maybe_contextmanager(timer, 'captcha-new', url):
-            response = cls._open(url)
-            if response.is_error:
-                raise NewCaptchaError(response.traceback, cls(None, None))
-            data = response.data()
-
-        m = re.search(r"challenge\s*:\s*'(.+?)'", data, re.M | re.S)
-        if m:
-            captcha_id = m.group(1)
-            image_url = settings.CAPTCHA_IMAGE_URL_PATTERN % captcha_id
-        else:
-            captcha_id, image_url = None, None
-        return cls(captcha_id, image_url, response)
-
-    def verify(self, captcha_solution, remote_ip, email, timer=None):
-        if self._verified is not None:
-            return self._verified
-
-        if self.check_whitelist(email):
-            return True
-
-        if not settings.CAPTCHA_PRIVATE_KEY:
-            raise EmptyCaptchaKey(
-                'Captcha.verify: CAPTCHA_PRIVATE_KEY setting is unset.')
-
-        if isinstance(captcha_solution, unicode):
-            captcha_solution = captcha_solution.encode('utf-8')
-
-        request_data = urllib.urlencode({
-            'privatekey': settings.CAPTCHA_PRIVATE_KEY,
-            'remoteip': remote_ip,
-            'challenge': self.captcha_id,
-            'response': captcha_solution,
-        })
-        request = urllib2.Request(settings.CAPTCHA_VERIFY_URL, request_data)
-        with maybe_contextmanager(timer, 'captcha-verify',
-                                  settings.CAPTCHA_VERIFY_URL):
-            self.response = self._open(request)
-
-        if not self.response.is_error:
-            response_data = self.response.data()
-            self.verified, self.message = response_data.split('\n', 1)
-            self._verified = self.verified.lower() == 'true'
-        elif self.captcha_id is None:
-            self.message = 'no-challenge'
-            self._verified = False
-        else:
-            self._verified = False
-            raise VerifyCaptchaError(self.response)
-        return self._verified
-
-
 class CaptchaV2(CaptchaBase):
     """Class to capture & abstract interaction with Google reCaptcha v2.0."""
 
@@ -247,13 +123,6 @@
         with maybe_contextmanager(timer, 'captcha-new', None):
             super(CaptchaV2, self).__init__()
 
-    @classmethod
-    def new(cls, timer=None):
-        """This method is kept for compatibility with the Captcha v1 class."""
-
-        cls._setup_opener()
-        return cls(timer)
-
     def verify(self, captcha_solution, remote_ip, email, timer=None):
         if self._verified is not None:
             return self._verified
@@ -263,7 +132,7 @@
 
         if not settings.CAPTCHA_PRIVATE_KEY:
             raise EmptyCaptchaKey(
-                'Captcha.verify: CAPTCHA_PRIVATE_KEY setting is unset.')
+                'CaptchaV2.verify: CAPTCHA_PRIVATE_KEY setting is unset.')
 
         if isinstance(captcha_solution, unicode):
             captcha_solution = captcha_solution.encode('utf-8')

=== modified file 'src/identityprovider/tests/test_models_captcha.py'
--- src/identityprovider/tests/test_models_captcha.py	2016-03-10 20:24:54 +0000
+++ src/identityprovider/tests/test_models_captcha.py	2018-10-25 17:56:53 +0000
@@ -3,19 +3,15 @@
 
 from contextlib import contextmanager
 from cStringIO import StringIO
-from unittest import skip
-from urllib2 import HTTPError, URLError
 
 from django.conf import settings
 from django.test import TestCase
 from django.test.utils import override_settings
-from mock import Mock, patch
+from mock import Mock
 
 from identityprovider.models.captcha import (
-    Captcha,
     CaptchaResponse,
     CaptchaV2,
-    NewCaptchaError,
     VerifyCaptchaError,
 )
 from identityprovider.tests.utils import SSOBaseTestCase
@@ -39,32 +35,6 @@
         self.assertEqual(r.data(), 'this is the data')
 
 
-class CaptchaOpenTestCase(TestCase):
-
-    def test_open_properly_handles_all_expected_error_codes(self):
-        handled_codes = [111, 113, 408, 500, 502, 503, 504]
-        Captcha._setup_opener()
-        self.addCleanup(setattr, Captcha, 'opener', None)
-
-        def build_mock_open(code):
-            def mock_open(request):
-                if code < 200:
-                    raise URLError([code, "reason"])
-                else:
-                    raise HTTPError(
-                        None, code, "reason", None, StringIO("test"))
-            return mock_open
-
-        for code in handled_codes:
-            Captcha.opener.open = build_mock_open(code)
-            response = Captcha._open(None)
-            self.assertTrue(response.is_error)
-            self.assertEqual(response.reason, "reason")
-            self.assertEqual(response.code, code)
-            if code >= 200:
-                self.assertEqual(response.body, "test")
-
-
 @override_settings(CAPTCHA_PUBLIC_KEY='public', CAPTCHA_PRIVATE_KEY='private')
 class CaptchaBaseTestCase(SSOBaseTestCase):
 
@@ -78,71 +48,6 @@
             return_value=self.captcha_response)
 
 
-class CaptchaTestCase(CaptchaBaseTestCase):
-
-    def test_opener_is_created_during_init(self):
-        Captcha.opener = None
-        Captcha('test')
-        self.assertTrue(Captcha.opener is not None)
-
-    @patch('urllib.urlencode')
-    def test_non_ascii_captcha_solution(self, mock_urlencode):
-        captcha_solution = u'\xa3'
-        encoded_solution = captcha_solution.encode('utf-8')
-        mock_urlencode.return_value = 'some text'
-
-        captcha = Captcha.new()
-        self.mock_captcha_open.return_value.is_error = True
-        captcha.verify(captcha_solution, 'foobar', '')
-
-        captcha_response = mock_urlencode.call_args[0][0]['response']
-        self.assertEqual(captcha_response, encoded_solution)
-
-    def test_opener_is_created_when_proxy_is_required(self):
-        Captcha.opener = None
-
-        with self.settings(CAPTCHA_PROXIES={'http': 'https://foo.com'}):
-            Captcha('test')
-
-        self.assertTrue(Captcha.opener is not None)
-
-    def test_serialize(self):
-        captcha = Captcha('id', 'image')
-
-        self.assertEqual(captcha.serialize(),
-                         {'captcha_id': 'id', 'image_url': 'image'})
-
-    def test_new_captcha_error(self):
-        self.captcha_response.is_error = True
-        with self.assertRaises(NewCaptchaError) as ctx:
-            Captcha.new()
-
-        e = ctx.exception
-        self.assertEqual(e.dummy.captcha_id, None)
-        self.assertEqual(e.dummy.image_url, None)
-
-    def test_captcha_open(self):
-        self.mock_captcha_open.side_effect = URLError((-1, 'error'))
-        self.assertRaises(URLError, Captcha.new)
-
-    def test_new_captcha_uses_timer(self):
-        passed_info = []
-
-        @contextmanager
-        def timer_fn(category, detail):
-            passed_info.append((category, detail))
-            yield
-
-        self.captcha_response.code = 200
-
-        Captcha.new(timer=timer_fn)
-
-        self.assertEqual(1, len(passed_info))
-        self.assertEqual('captcha-new', passed_info[0][0])
-        self.assertEqual(
-            self.mock_captcha_open.call_args[0][0], passed_info[0][1])
-
-
 class CaptchaV2TestCase(CaptchaBaseTestCase):
 
     def test_opener_is_created_during_init(self):
@@ -178,14 +83,10 @@
         self.assertEqual(settings.CAPTCHA_V2_VERIFY_URL, passed_info[0][1])
 
 
-class CaptchaVerifyTestCase(CaptchaBaseTestCase):
-
-    CAPTCHA = Captcha
-    VERIFICATION_URL = settings.CAPTCHA_VERIFY_URL
-    RESPONSE_OK = 'true\nok'
+class CaptchaV2VerifyTestCase(CaptchaBaseTestCase):
 
     def test_verify_calls_check_whitelist(self):
-        captcha = self.CAPTCHA(None)
+        captcha = CaptchaV2(None)
         captcha.check_whitelist = Mock(return_value=False)
 
         result = captcha.verify(None, 'localhost', 'foo')
@@ -195,7 +96,7 @@
         captcha.check_whitelist.assert_called_with('foo')
 
     def test_check_whitelist_match(self):
-        captcha = self.CAPTCHA(None)
+        captcha = CaptchaV2(None)
 
         regexps = ['not a match', '^foo$']
         with self.settings(EMAIL_WHITELIST_REGEXP_LIST=regexps):
@@ -204,7 +105,7 @@
             self.assertFalse(self.mock_captcha_open.called)
 
     def test_check_whitelist_no_match(self):
-        captcha = self.CAPTCHA(None)
+        captcha = CaptchaV2(None)
 
         regexps = ['not a match', '^bar$']
         with self.settings(EMAIL_WHITELIST_REGEXP_LIST=regexps):
@@ -213,28 +114,21 @@
             self.assertTrue(self.mock_captcha_open.called)
 
     def test_verify_already_verified(self):
-        c = self.CAPTCHA(None)
+        c = CaptchaV2(None)
         c._verified = True
         r = c.verify(None, None, '')
         self.assertTrue(r)
 
     def test_verify_response_ok(self):
         self.captcha_response.code = 200
-        self.captcha_response.response = StringIO(self.RESPONSE_OK)
+        self.captcha_response.response = StringIO('{"success": true}')
 
-        c = self.CAPTCHA.new()
+        c = CaptchaV2()
         self.assertTrue(c.verify(None, "localhost", ''))
         self.assertEqual(c.message, 'ok')
 
-    def test_verify_no_captcha_id(self):
-        c = self.CAPTCHA(None)
-
-        self.captcha_response.is_error = True
-        self.assertFalse(c.verify(None, 'localhost', ''))
-        self.assertEqual(c.message, 'no-challenge')
-
     def test_verify_error(self):
-        c = self.CAPTCHA(None)
+        c = CaptchaV2(None)
         c.captcha_id = 'challenge-id'
 
         self.captcha_response.is_error = True
@@ -247,18 +141,8 @@
         def timer_fn(category, detail):
             passed_info.append((category, detail))
             yield
-        c = self.CAPTCHA(None)
+        c = CaptchaV2(None)
         c.verify(None, 'localhost', '', timer=timer_fn)
         self.assertEqual('captcha-verify', passed_info[0][0])
-        self.assertEqual(self.VERIFICATION_URL, passed_info[0][1])
-
-
-class CaptchaV2VerifyTestCase(CaptchaVerifyTestCase):
-
-    CAPTCHA = CaptchaV2
-    VERIFICATION_URL = settings.CAPTCHA_V2_VERIFY_URL
-    RESPONSE_OK = '{"success": true}'
-
-    @skip("This (inherited) test makes no sense for CaptchaV2")
-    def test_verify_no_captcha_id(self):
-        pass
+        self.assertEqual(
+            settings.CAPTCHA_V2_VERIFY_URL, passed_info[0][1])

=== modified file 'src/mockservice/sso_mockserver/wadl.xml'
--- src/mockservice/sso_mockserver/wadl.xml	2012-12-18 18:37:03 +0000
+++ src/mockservice/sso_mockserver/wadl.xml	2018-10-25 17:56:53 +0000
@@ -564,7 +564,7 @@
                         required="true" fixed="register">
               <wadl:doc>The name of the operation being invoked.</wadl:doc>
             </wadl:param>
-            <wadl:param style="query" required="true"
+            <wadl:param style="query" required="false"
                         name="captcha_solution">
              <wadl:doc xmlns="http://www.w3.org/1999/xhtml">
 Solution for the generated captcha.
@@ -578,7 +578,7 @@
 </wadl:doc>
               
             </wadl:param>
-            <wadl:param style="query" required="true"
+            <wadl:param style="query" required="false"
                         name="captcha_id">
              <wadl:doc xmlns="http://www.w3.org/1999/xhtml">
 ID for the generated captcha

=== modified file 'src/webui/tests/test_views_registration.py'
--- src/webui/tests/test_views_registration.py	2018-10-25 17:56:53 +0000
+++ src/webui/tests/test_views_registration.py	2018-10-25 17:56:53 +0000
@@ -582,6 +582,7 @@
         self.assertEqual(response.status_code, 405)
 
 
+@switches(TWOFACTOR=True)
 class TwoFactorSyncTestCase(SSOBaseTestCase):
     """Tests for 2fa OTP synchronization."""
 
@@ -605,7 +606,9 @@
             email = self.email
         data = {'email': email, 'password': DEFAULT_USER_PASSWORD}
         url = reverse('login')
-        return self.client.post(url, data)
+        response = self.client.post(url, data)
+        self.assertRedirects(response, reverse('account-index'))
+        return response
 
     def test_shows_all_fields_for_synchronization(self):
         self.factory.make_account(email=self.email)