Hi Michael. Excellent branch... the implementation and tests are all very clear.

The only potential danger I can see with this branch is the ability to add unsafe regexes, but I don't see anyway around that unless we restricted ourselves to gmail accounts (ie. usesd settings.GMAIL_IDENTIFIER_WHITELIST and automatically calculated the regex to allow identifier+whatever).

The only questions I've got below are for my own understanding or tiny style questions. I verified that all 611 tests pass.

> === modified file 'identityprovider/models/captcha.py'
> --- identityprovider/models/captcha.py	2011-06-30 04:55:13 +0000
> +++ identityprovider/models/captcha.py	2011-07-26 07:31:31 +0000
> @@ -190,3 +194,9 @@
>              self._verified = False
>              raise VerifyCaptchaError(self.response.traceback)
>          return self._verified
> +
> +    def check_whitelist(self, email):
> +        for pattern in settings.EMAIL_WHITELIST_REGEXP_LIST:
> +            if re.match(pattern, email):
> +                return True
> +        return False

So that's the crux of the branch right? Nice and simple :)

The only thought I had was whether it should be *outside* of captcha.verify(),
rather than being hidden within, but that's just a small point - nothing to
delay the branch landing over :)

>
> === modified file 'identityprovider/schema.py'
> --- identityprovider/schema.py	2011-07-12 09:41:08 +0000
> +++ identityprovider/schema.py	2011-07-26 07:31:31 +0000
> @@ -78,6 +78,9 @@
>      captcha.captcha_public_key = StringConfigOption()
>      captcha.captcha_private_key = StringConfigOption()
>      captcha.disable_captcha_verification = BoolConfigOption()
> +    captcha.email_whitelist_regexp_list = LinesConfigOption(
> +        item=StringConfigOption(raw=True)

I wasn't sure at first why the raw=True was necessary, until I tried
repr('^canonicaltest(?:\+.+)?@gmail\.com$').

> +    )
>
>      # debug
>      debug = ConfigSection()
>
> === modified file 'identityprovider/tests/test_auth.py'
> --- identityprovider/tests/test_auth.py	2011-07-15 15:17:19 +0000
> +++ identityprovider/tests/test_auth.py	2011-07-26 07:31:31 +0000

Great to fix up all the patch_object's :)

> === modified file 'identityprovider/tests/test_captcha.py'
> --- identityprovider/tests/test_captcha.py	2011-07-21 16:08:06 +0000
> +++ identityprovider/tests/test_captcha.py	2011-07-26 07:31:31 +0000
> === modified file 'identityprovider/tests/test_command_cleanup.py'
> --- identityprovider/tests/test_command_cleanup.py	2011-07-15 11:50:53 +0000
> +++ identityprovider/tests/test_command_cleanup.py	2011-07-26 07:31:31 +0000
> === modified file 'identityprovider/tests/test_forms.py'
> --- identityprovider/tests/test_forms.py	2011-07-15 11:50:53 +0000
> +++ identityprovider/tests/test_forms.py	2011-07-26 07:31:31 +0000
> @@ -18,6 +18,7 @@
>  )
>  from identityprovider.api10.forms import WebserviceCreateAccountForm
>  from identityprovider.models.openidmodels import OpenIDRPConfig
> +from identityprovider.tests.utils import patch_settings
>
>  from .utils import SSOBaseTestCase
>
> @@ -114,6 +115,23 @@
>          self.assertTrue(form.is_valid())
>          self.assertEqual(form.cleaned_data['platform'], 'desktop')
>
> +    def test_captcha_checked_for_whitelist(self):
> +        data = {
> +            'email': '<email address hidden>',
> +            'password': 'password1A',
> +            'captcha_id': '1',
> +            'captcha_solution': '2',
> +            'remote_ip': '127.0.0.1',
> +        }
> +        pattern = '^canonicaltest(?:\+.+)?@gmail\.com$'
> +        overrides = dict(
> +            DISABLE_CAPTCHA_VERIFICATION=False,
> +            EMAIL_WHITELIST_REGEXP_LIST=[pattern],
> +        )
> +        with patch_settings(**overrides):
> +            form = WebserviceCreateAccountForm(data)
> +            self.assertTrue(form.is_valid())
> +

Great.

>      def test_default_cleaned_validate_redirect_to(self):
>          data = {
>              'email': '<email address hidden>',
>
> === modified file 'identityprovider/tests/test_models_captcha.py'
> --- identityprovider/tests/test_models_captcha.py	2011-01-20 21:48:43 +0000
> +++ identityprovider/tests/test_models_captcha.py	2011-07-26 07:31:31 +0000
> @@ -70,23 +75,56 @@
>      def tearDown(self):
>          settings.DISABLE_CAPTCHA_VERIFICATION = self.old_DISABLE_CAPTCHA_VERIFICATION
>
> -    @patch_object(Captcha, '_open')
> +    @patch.object(Captcha, '_open')
> +    def test_verify_calls_check_whitelist(self, mock_open):
> +        captcha = Captcha(None)
> +        captcha.check_whitelist = Mock(return_value=False)
> +
> +        result = captcha.verify(None, 'localhost', 'foo')
> +        self.assertFalse(result)
> +        self.assertTrue(mock_open.called)
> +
> +        captcha.check_whitelist.assert_called_with('foo')

Great to have the above, but I couldn't see why it was necessary given the
two tests below.

> +
> +    @patch.object(Captcha, '_open')
> +    def test_check_whitelist_match(self, mock_open):
> +        captcha = Captcha(None)
> +
> +        regexps = ['not a match', '^foo$']
> +        with patch_settings(
> +                EMAIL_WHITELIST_REGEXP_LIST=regexps
> +            ):
> +            result = captcha.verify(None, 'localhost', 'foo')
> +            self.assertTrue(result)
> +            self.assertFalse(mock_open.called)
> +
> +    @patch.object(Captcha, '_open')
> +    def test_check_whitelist_no_match(self, mock_open):
> +        captcha = Captcha(None)
> +
> +        regexps = ['not a match', '^bar$']
> +        with patch_settings(
> +                EMAIL_WHITELIST_REGEXP_LIST=regexps
> +            ):
> +            result = captcha.verify(None, 'localhost', 'foo')
> +            self.assertFalse(result)
> +            self.assertTrue(mock_open.called)
> +
> +    @patch.object(Captcha, '_open')
>      def test_verify_already_verified(self, mock_open):
>          c = Captcha(None)
>          c._verified = True
> @@ -94,25 +132,28 @@
>          old_open = Captcha._open
>          Captcha._open = mock_open
>
> -        c = Captcha.new()
> -        r = c.verify(None, "localhost")
> -
> -        self.assertTrue(r)
> -        self.assertEqual(c.message, 'ok')
> -
> -        Captcha._open = old_open
> -
> -    @patch_object(Captcha, '_open')
> +        try:
> +            c = Captcha.new()
> +            r = c.verify(None, "localhost", '')
> +
> +            self.assertTrue(r)
> +            self.assertEqual(c.message, 'ok')
> +        finally:
> +            Captcha._open = old_open

I don't see why you don't just use `with patch.object(Captcha, '_open')...`
above. I'm sure you had a reason, I'm just keen to understand what it was :)

> === modified file 'identityprovider/tests/test_views_ui.py'
> --- identityprovider/tests/test_views_ui.py	2011-07-19 14:21:42 +0000
> +++ identityprovider/tests/test_views_ui.py	2011-07-26 07:31:31 +0000
> @@ -815,6 +816,55 @@
>          msg = 'It appears that our captcha service was unable to load'
>          self.assertContains(r, msg)
>
> +    def test_new_account_captcha_whitelist(self):
> +        email = '<email address hidden>'
> +        pattern = '^canonicaltest(?:\+.+)?@gmail\.com$'
> +        overrides = dict(
> +            DISABLE_CAPTCHA_VERIFICATION=False,
> +            EMAIL_WHITELIST_REGEXP_LIST=[pattern],
> +        )
> +        with patch_settings(**overrides):
> +            response = _post_new_account(
> +                self.client,
> +                email=email
> +            )
> +            # should redirect to '/+email-sent'
> +            self.assertEqual(response.status_code, 302)
> +            redirect = response['location']
> +            self.assertTrue(redirect.endswith('/+email-sent'))
> +
> +    def test_new_account_captcha_whitelist_with_uuid(self):
> +        email = '<email address hidden>'
> +        pattern = '^canonicaltest(?:\+.+)?@gmail\.com$'
> +        overrides = dict(
> +            DISABLE_CAPTCHA_VERIFICATION=False,
> +            EMAIL_WHITELIST_REGEXP_LIST=[pattern],
> +        )
> +        with patch_settings(**overrides):
> +            response = _post_new_account(
> +                self.client,
> +                email=email
> +            )
> +            # should redirect to '/+email-sent'
> +            self.assertEqual(response.status_code, 302)
> +            redirect = response['location']
> +            self.assertTrue(redirect.endswith('/+email-sent'))

Great that you added a test specifically for the +something case (even though
it's not part of the functionality of the code, but *is* the regex we'll be
using).

> +
> +    def test_new_account_captcha_whitelist_fail(self):
> +        email = '<email address hidden>'
> +        pattern = '^canonicaltest(?:\+.+)?@gmail\.com$'
> +        overrides = dict(
> +            DISABLE_CAPTCHA_VERIFICATION=False,
> +            EMAIL_WHITELIST_REGEXP_LIST=[pattern],
> +        )
> +        with patch_settings(**overrides):
> +            response = _post_new_account(
> +                self.client,
> +                email=email
> +            )
> +            msg = 'It appears that our captcha service was unable to load'
> +            self.assertContains(response, msg)
> +
>
>  class LanguagesTestCase(SSOBaseTestCase):
>
>
> === modified file 'identityprovider/views/ui.py'
> --- identityprovider/views/ui.py	2011-07-21 16:08:06 +0000
> +++ identityprovider/views/ui.py	2011-07-26 07:31:31 +0000
> @@ -464,9 +464,10 @@
>  def _verify_captcha_response(template, request, form):
>      captcha = Captcha(request.POST.get('recaptcha_challenge_field'))
>      captcha_solution = request.POST.get('recaptcha_response_field')
> +    email = request.POST.get('email', '')

At first this worried me, but then I saw that _verify_captcha_response is only
used within the new_account and forgot_password views where (1) there is no
corresponding model known, and (2) the functionality is dependent on the user
having access to the email address. Great.

>      ip_addr = request.environ["REMOTE_ADDR"]
>      try:
> -        verified = captcha.verify(captcha_solution, ip_addr)
> +        verified = captcha.verify(captcha_solution, ip_addr, email)
>          if verified:
>              return None
>      except VerifyCaptchaError, e:
>
> === modified file 'requirements.txt'
> --- requirements.txt	2011-07-12 14:38:02 +0000
> +++ requirements.txt	2011-07-26 07:31:31 +0000
> @@ -6,7 +6,7 @@
>  NoseDjango==0.8.1
>  nosexcover
>  coverage==3.4
> -mock==0.6.0
> +mock==0.7.2
>  wsgiref==0.1.2
>  wsgi-intercept==0.4
>  zope.testbrowser==3.5.1
>
> === modified file 'setup.py'
> --- setup.py	2011-07-06 17:36:56 +0000
> +++ setup.py	2011-07-26 07:31:31 +0000
> @@ -39,7 +39,7 @@
>      'NoseDjango==0.8.1',
>      'nosexcover',
>      'coverage==3.4',
> -    'mock==0.6.0',
> +    'mock==0.7.2',

Why is this even in the setup.py? I thought (probably wrongly) that mock or
nose or wsgi-intercept needed to be in the requirements.txt for setting up the dev environment,
but not in the setup.py, as there's no need for them to be installed? I assume
that's wrong then (on my part)? Do python packages install their test
dependencies so they can be tested after install? (is that possible with our
packages?)

Thanks!

>      'wsgiref==0.1.2',
>      'wsgi-intercept==0.4',
>      'zope.testbrowser==3.5.1',
>