Merge lp:~bac/launchpad/bug-452491-captcha2-boogaloo into lp:launchpad

[Brad Crittenden]

Add simple captcha to the +forgottenpassword page.

[Brad Crittenden (bac)]

= Summary =

Bug 452491 requests a simple captcha be placed on the forgotten password page.

== Proposed fix ==

The captcha already existed on the registration page. It was quick work to refactor
the bits into a mixin class and let the forgotten password page use it. Sadly both
of those pages are done without using LaunchpadFormView, made instead with grout and
twine, so it took a little massaging to get things to work.

The test helper set_captcha_answer had to be made a little smarter to account for
form prefixes.

The reset password test needed some clean up too. There is a fair amount of drive by
stuff in here but it's easy to sort out, I hope.

I also changed the spacing on the error messages for this page and the registration
page to fix some overlap problems. Screenshots are at:
http://people.canonical.com/~bac/captcha/

== Pre-implementation notes ==

None

== Implementation details ==

As above.

== Tests ==

bin/test -vvm lp.registry -t foaf -t xx-new-account-redirection-url.txt

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/templates/launchpad-login.pt
  lib/canonical/launchpad/templates/launchpad-forgottenpassword.pt
  lib/lp/registry/stories/foaf/xx-reg-with-existing-email.txt
  lib/lp/registry/stories/foaf/xx-createaccount.txt
  lib/canonical/launchpad/browser/tests/registration.py
  lib/canonical/launchpad/webapp/login.py
  lib/lp/testing/registration.py
  lib/canonical/launchpad/pagetests/standalone/xx-new-account-redirection-url.txt
  lib/lp/registry/stories/foaf/xx-resetpassword.txt

[Abel Deuring (adeuring)]

Hi Brad,

a nice branch r=me. I have only a minor cosmetic suggestion, see below.

Abel

> === modified file 'lib/lp/testing/registration.py'
> --- lib/lp/testing/registration.py 2009-10-08 20:22:25 +0000
> +++ lib/lp/testing/registration.py 2009-10-16 16:35:20 +0000
> @@ -24,9 +24,10 @@
> return ''
>
>
> -def set_captcha_answer(browser, answer=None):
> +def set_captcha_answer(browser, answer=None, prefix=''):
> """Given a browser, set the login captcha with the correct answer."""
> if answer is None:
> answer = get_captcha_answer(browser.contents)
> - browser.getControl(name='loginpage_captcha_submission').value = (
> + control_name = prefix + 'captcha_submission'
> + browser.getControl(name=control_name).value = (
> answer)

I think you can merge the last two lines in one ;)

review: Approve

[Brad Crittenden (bac)]

> Hi Brad,
>
> a nice branch r=me. I have only a minor cosmetic suggestion, see below.
>
> Abel
>
> > === modified file 'lib/lp/testing/registration.py'
> > --- lib/lp/testing/registration.py 2009-10-08 20:22:25 +0000
> > +++ lib/lp/testing/registration.py 2009-10-16 16:35:20 +0000
> > @@ -24,9 +24,10 @@
> > return ''
> >
> >
> > -def set_captcha_answer(browser, answer=None):
> > +def set_captcha_answer(browser, answer=None, prefix=''):
> > """Given a browser, set the login captcha with the correct answer."""
> > if answer is None:
> > answer = get_captcha_answer(browser.contents)
> > - browser.getControl(name='loginpage_captcha_submission').value = (
> > + control_name = prefix + 'captcha_submission'
> > + browser.getControl(name=control_name).value = (
> > answer)
>
> I think you can merge the last two lines in one ;)

Done

[Martin Albisetti (beuno)]

Bonus points if the forms look like everywhere else in Launchpad.

review: Approve (ui)

=== modified file 'lib/canonical/launchpad/browser/tests/registration.py'
--- lib/canonical/launchpad/browser/tests/registration.py	2009-10-08 19:52:06 +0000
+++ lib/canonical/launchpad/browser/tests/registration.py	2009-10-16 19:59:17 +0000
@@ -21,7 +21,7 @@
     browser = setupBrowser()
     browser.open('http://launchpad.dev/+login')
     browser.getControl(name='loginpage_email', index=1).value = email
-    set_captcha_answer(browser)
+    set_captcha_answer(browser, prefix='loginpage_')
     browser.getControl('Register').click()
     return browser
 

=== modified file 'lib/canonical/launchpad/pagetests/standalone/xx-new-account-redirection-url.txt'
--- lib/canonical/launchpad/pagetests/standalone/xx-new-account-redirection-url.txt	2009-10-08 19:52:06 +0000
+++ lib/canonical/launchpad/pagetests/standalone/xx-new-account-redirection-url.txt	2009-10-16 19:59:17 +0000
@@ -42,7 +42,7 @@
     >>> from lp.testing.registration import set_captcha_answer
     >>> anon_browser.getControl('E-mail address:', index=1).value = (
     ...     '[e-mail]')
-    >>> set_captcha_answer(anon_browser)
+    >>> set_captcha_answer(anon_browser, prefix='loginpage_')
     >>> anon_browser.getControl('Register').click()
     >>> print anon_browser.contents
     <...

=== modified file 'lib/canonical/launchpad/templates/launchpad-forgottenpassword.pt'
--- lib/canonical/launchpad/templates/launchpad-forgottenpassword.pt	2009-07-17 17:59:07 +0000
+++ lib/canonical/launchpad/templates/launchpad-forgottenpassword.pt	2009-10-16 19:59:17 +0000
@@ -21,22 +21,49 @@
 
     <div tal:condition="not: view/success">
 
-      <div class="error" tal:condition="view/errortext"
-        tal:content="structure view/errortext" />
+      <tal:block condition="view/errortext">
+        <h1>Your password reset request was unsuccessful</h1>
+        <p class="error message"
+           style="margin-top: 2em;"
+           tal:content="structure view/errortext" />
+      </tal:block>
 
       <p>
         Enter your e-mail address, and we will send you instructions
-        on how to reset your password. 
+        on how to reset your password.  You'll also need to answer a simple
+        question so we know that you're human.
       </p>
 
       <form name="forgottenpassword" method="POST">
+        <input type="hidden"
+               tal:attributes="name view/captcha_hash;
+                               value view/get_captcha_hash" />
       <div class="field">
-        <label for="email">Email address:</label>
-        <input type="text" size="50" name="email" />
+        <table>
+          <tr>
+            <th>
+              <label for="email">Email address:</label>
+            </th>
+            <td>
+              <input type="text" size="50" name="email" />
+            </td>
+          </tr>
+          <tr>
+            <th>
+              <label>Random question:</label>
+            </th>
+            <td>
+              <span id="problem" tal:content="view/captcha_problem" />
+              <input type="text" size="5"
+                     id="captcha_submission" value=""
+                     tal:attributes="name view/captcha_submission"
+                     />
+            </td>
+          </tr>
+        </table>
         <div class="formHelp">
           <p>
-          This should be the email address registered for your Launchpad
-          account.
+          Enter the email address registered for your Launchpad account.
           </p>
           <p>
           If you are claiming an existing account but don't know the email

=== modified file 'lib/canonical/launchpad/templates/launchpad-login.pt'
--- lib/canonical/launchpad/templates/launchpad-login.pt	2009-10-08 15:57:01 +0000
+++ lib/canonical/launchpad/templates/launchpad-login.pt	2009-10-16 19:59:17 +0000
@@ -164,6 +164,7 @@
             <tal:block condition="view/registration_error">
               <h1>Your registration was unsuccessful</h1>
               <p class="error message"
+                 style="margin-top: 2em;"
                  tal:content="structure view/registration_error" />
             </tal:block>
             <!-- 5. might want to register: -->

=== modified file 'lib/canonical/launchpad/webapp/login.py'
--- lib/canonical/launchpad/webapp/login.py	2009-10-08 15:57:01 +0000
+++ lib/canonical/launchpad/webapp/login.py	2009-10-16 19:59:17 +0000
@@ -153,8 +153,44 @@
         return getUtility(IPersonSet).getByName(
             config.launchpad.restrict_to_team).title
 
-
-class LoginOrRegister:
+class CaptchaMixin:
+    """Mixin class to provide simple captcha capabilities."""
+    def validateCaptcha(self):
+        """Validate the submitted captcha value matches what we expect."""
+        expected = self.request.form.get(self.captcha_hash)
+        submitted = self.request.form.get(self.captcha_submission)
+        if expected is not None and submitted is not None:
+            return md5.new(submitted).hexdigest() == expected
+        return False
+
+    @cachedproperty
+    def captcha_answer(self):
+        """Get the answer for the current captcha challenge.
+
+        With each failed attempt a new challenge will be given.  Our answer
+        space is acknowledged to be ridiculously small but is chosen in the
+        interest of ease-of-use.  We're not trying to create an iron-clad
+        challenge but only a minimal obstacle to dumb bots.
+        """
+        return random.randint(10, 20)
+
+    @property
+    def get_captcha_hash(self):
+        """Get the captcha hash.
+
+        The hash is the value we put in the form for later comparison.
+        """
+        return md5.new(str(self.captcha_answer)).hexdigest()
+
+    @property
+    def captcha_problem(self):
+        """Create the captcha challenge."""
+        op1 = random.randint(1, self.captcha_answer - 1)
+        op2 = self.captcha_answer - op1
+        return '%d + %d =' % (op1, op2)
+
+
+class LoginOrRegister(CaptchaMixin):
     """Merges the former CookieLoginPage and JoinLaunchpadView classes
     to allow the two forms to appear on a single page.
 
@@ -399,41 +435,6 @@
             L.append(html % (name, cgi.escape(value, quote=True)))
         return '\n'.join(L)
 
-    def validateCaptcha(self):
-        """Validate the submitted captcha value matches what we expect."""
-        expected = self.request.form.get(self.captcha_hash)
-        submitted = self.request.form.get(self.captcha_submission)
-        if expected is not None and submitted is not None:
-            return md5.new(submitted).hexdigest() == expected
-        return False
-
-    @cachedproperty
-    def captcha_answer(self):
-        """Get the answer for the current captcha challenge.
-
-        With each failed attempt a new challenge will be given.  Our answer
-        space is acknowledged to be ridiculously small but is chosen in the
-        interest of ease-of-use.  We're not trying to create an iron-clad
-        challenge but only a minimal obstacle to dumb bots.
-        """
-        return random.randint(10, 20)
-
-    @property
-    def get_captcha_hash(self):
-        """Get the captcha hash.
-
-        The hash is the value we put in the form for later comparison.
-        """
-        return md5.new(str(self.captcha_answer)).hexdigest()
-
-    @property
-    def captcha_problem(self):
-        """Create the captcha challenge."""
-        op1 = random.randint(1, self.captcha_answer)
-        op2 = self.captcha_answer - op1
-        return '%d + %d =' % (op1, op2)
-
-
 def logInPrincipal(request, principal, email):
     """Log the principal in. Password validation must be done in callsites."""
     session = ISession(request)
@@ -542,16 +543,25 @@
         return ''
 
 
-class ForgottenPasswordPage:
+class ForgottenPasswordPage(CaptchaMixin):
 
     errortext = None
     submitted = False
+    captcha_submission = 'captcha_submission'
+    captcha_hash = 'captcha_hash'
 
     def process_form(self):
         request = self.request
         if request.method != "POST":
             return
 
+        # Validate the user is human, more or less.
+        if not self.validateCaptcha():
+            self.errortext = (
+                "The answer to the simple math question was incorrect "
+                "or missing.  Please try again.")
+            return
+
         email = request.form.get("email").strip()
         person = getUtility(IPersonSet).getByEmail(email)
         if person is None:

=== modified file 'lib/lp/registry/stories/foaf/xx-createaccount.txt'
--- lib/lp/registry/stories/foaf/xx-createaccount.txt	2009-10-08 19:52:06 +0000
+++ lib/lp/registry/stories/foaf/xx-createaccount.txt	2009-10-16 19:59:17 +0000
@@ -36,7 +36,7 @@
 email address from before has been retained in the form.
 
     >>> from lp.testing.registration import set_captcha_answer
-    >>> set_captcha_answer(browser)
+    >>> set_captcha_answer(browser, prefix='loginpage_')
     >>> browser.getControl('Register').click()
     >>> print_feedback_messages(browser.contents)
     >>> print extract_text(find_main_content(browser.contents))
@@ -103,7 +103,7 @@
     >>> browser.open('http://launchpad.dev/+login')
     >>> browser.getControl(name='loginpage_email', index=1).value = (
     ...     '[e-mail]')
-    >>> set_captcha_answer(browser)
+    >>> set_captcha_answer(browser, prefix='loginpage_')
     >>> browser.getControl('Register').click()
 
     >>> from_addr, to_addrs, raw_msg = stub.test_emails.pop()

=== modified file 'lib/lp/registry/stories/foaf/xx-reg-with-existing-email.txt'
--- lib/lp/registry/stories/foaf/xx-reg-with-existing-email.txt	2009-10-08 19:52:06 +0000
+++ lib/lp/registry/stories/foaf/xx-reg-with-existing-email.txt	2009-10-16 19:59:17 +0000
@@ -16,7 +16,7 @@
     >>> browser.getControl('E-mail address:', index=1).value = (
     ...     '[e-mail]')
     >>> from lp.testing.registration import set_captcha_answer
-    >>> set_captcha_answer(browser)
+    >>> set_captcha_answer(browser, prefix='loginpage_')
     >>> browser.getControl('Register').click()
 
     >>> for message in get_feedback_messages(browser.contents):
@@ -33,7 +33,7 @@
 on with the registration process.
 
     >>> browser.getControl('E-mail address:', index=1).value = '[e-mail]'
-    >>> set_captcha_answer(browser)
+    >>> set_captcha_answer(browser, prefix='loginpage_')
     >>> browser.getControl('Register').click()
 
     >>> print extract_text(find_tag_by_id(browser.contents, 'address'))
@@ -85,7 +85,7 @@
 
     >>> anon_browser.getControl('E-mail address:', index=1).value = (
     ...     '[e-mail]')
-    >>> set_captcha_answer(anon_browser)
+    >>> set_captcha_answer(anon_browser, prefix='loginpage_')
     >>> anon_browser.getControl('Register').click()
 
     >>> from_addr, to_addr, msg = stub.test_emails.pop()
@@ -147,7 +147,7 @@
 
     >>> browser.getControl('E-mail address:', index=1).value = (
     ...     '[e-mail]')
-    >>> set_captcha_answer(browser)
+    >>> set_captcha_answer(browser, prefix='loginpage_')
     >>> browser.getControl('Register').click()
 
     >>> print extract_text(find_main_content(browser.contents).p)

=== modified file 'lib/lp/registry/stories/foaf/xx-resetpassword.txt'
--- lib/lp/registry/stories/foaf/xx-resetpassword.txt	2009-09-18 01:09:10 +0000
+++ lib/lp/registry/stories/foaf/xx-resetpassword.txt	2009-10-16 19:59:17 +0000
@@ -15,10 +15,12 @@
     >>> browser.title
     'Need a new Launchpad password?'
 
-He types the email address registered in Launchpad and submit the form.
+He types the email address registered in Launchpad and submits the form.
 
+    >>> from lp.testing.registration import set_captcha_answer
     >>> browser.getControl(name='email').value = (
     ...     '[e-mail]')
+    >>> set_captcha_answer(browser)
     >>> browser.getControl('Request Reset').click()
     >>> print extract_text(find_main_content(browser.contents))
     Need a new Launchpad password?
@@ -68,8 +70,7 @@
     >>> browser.url
     'http://launchpad.dev/token/.../+resetpassword'
 
-    >>> for tag in get_feedback_messages(browser.contents):
-    ...     print extract_text(tag)
+    >>> print_feedback_messages(browser.contents)
     There is 1 error.
     The password provided contains non-ASCII characters.
 
@@ -82,8 +83,7 @@
     >>> browser.url
     'http://launchpad.dev'
 
-    >>> for tag in get_feedback_messages(browser.contents):
-    ...     print extract_text(tag)
+    >>> print_feedback_messages(browser.contents)
     Your password has been reset successfully.
 
     >>> print extract_text(find_tag_by_id(browser.contents, 'logincontrol'))
@@ -104,8 +104,7 @@
 logs out and then logs in again:
 
     >>> browser.open('http://launchpad.dev/+logout')
-    >>> for tag in get_feedback_messages(browser.contents):
-    ...     print extract_text(tag)
+    >>> print_feedback_messages(browser.contents)
     You have been logged out
 
     >>> browser.getLink('Log in / Register').click()
@@ -122,6 +121,20 @@
     David Allouche...
 
 
+== Reset is protected by a math captcha ==
+
+When requesting a password reset David is presented with a simple math
+problem which must be solved correctly before proceeding.
+
+    >>> browser.open('http://launchpad.dev/+forgottenpassword')
+    >>> browser.getControl(name='email').value = (
+    ...     '[e-mail]')
+    >>> browser.getControl(name='captcha_submission').value = '-1'
+    >>> browser.getControl('Request Reset').click()
+    >>> print_feedback_messages(browser.contents)
+    The answer to the simple math question was incorrect or missing. Please try again.
+
+
 == Using email addresses other than the preferred one ==
 
 Any of a person's validated email addresses can be used to reset his
@@ -134,8 +147,9 @@
     >>> browser.open('http://launchpad.dev/+login')
     >>> browser.getLink('Forgotten your password?').click()
     >>> browser.getControl(name='email').value = '[e-mail]'
+    >>> set_captcha_answer(browser)
     >>> browser.getControl('Request Reset').click()
-    >>> print "\n".join(get_feedback_messages(browser.contents))
+    >>> print_feedback_messages(browser.contents)
 
 He follows the link sent to his email just like he did before.
 
@@ -155,7 +169,7 @@
     >>> browser.getControl('Continue').click()
     >>> browser.url
     'http://launchpad.dev'
-    >>> print "\n".join(get_feedback_messages(browser.contents))
+    >>> print_feedback_messages(browser.contents)
     Your password has been reset successfully.
 
 
@@ -165,12 +179,11 @@
 
     >>> browser.open('http://launchpad.dev/+forgottenpassword')
     >>> browser.getControl(name='email').value = '[e-mail]'
+    >>> set_captcha_answer(browser)
     >>> browser.getControl('Request Reset').click()
-
-    >>> for tag in find_tags_by_class(browser.contents, 'error'):
-    ...     print tag
-    <div class="error">The email address <strong>[e-mail]</strong>
-    belongs to a team, and teams cannot log in to Launchpad.</div>
+    >>> print_feedback_messages(browser.contents)
+    The email address [e-mail]
+    belongs to a team, and teams cannot log in to Launchpad.
 
 
 == Reactivating an account ==
@@ -202,8 +215,7 @@
     >>> browser.title
     'Log in or register with Launchpad'
 
-    >>> for tag in get_feedback_messages(browser.contents):
-    ...     print extract_text(tag)
+    >>> print_feedback_messages(browser.contents)
     The email address belongs to a deactivated account. Use the
     "Forgotten your password" link to reactivate it.
 
@@ -215,6 +227,7 @@
 
     >>> browser.getControl(name='email').value = (
     ...     '[e-mail]')
+    >>> set_captcha_answer(browser)
     >>> browser.getControl('Request Reset').click()
 
     >>> print extract_text(find_main_content(browser.contents).p)
@@ -248,8 +261,7 @@
     >>> browser.url
     'http://launchpad.dev'
 
-    >>> for tag in get_feedback_messages(browser.contents):
-    ...     print extract_text(tag)
+    >>> print_feedback_messages(browser.contents)
     Welcome back to Launchpad.
     Your password has been reset successfully.
 
@@ -300,6 +312,7 @@
     'Need a new Launchpad password?'
     >>> browser.getControl(name='email').value = (
     ...     '[e-mail]')
+    >>> set_captcha_answer(browser)
     >>> browser.getControl('Request Reset').click()
 
     >>> print extract_text(find_main_content(browser.contents).p)
@@ -330,9 +343,6 @@
     >>> browser.getControl(name='field.password_dupe').value = 'test'
     >>> browser.getControl('Continue').click()
 
-    >>> for tag in find_tags_by_class(
-    ...     browser.contents, 'warning'):
-    ...     print tag
-    <div ...>Your password cannot be reset because your account is suspended.
-    Contact a <a href="mailto:[e-mail]?subject=SU...">Launchpad
-    admin</a> about this issue.</div>
+    >>> print_feedback_messages(browser.contents)
+    Your password cannot be reset because your account is suspended.
+    Contact a Launchpad admin about this issue.

=== modified file 'lib/lp/testing/registration.py'
--- lib/lp/testing/registration.py	2009-10-08 20:22:25 +0000
+++ lib/lp/testing/registration.py	2009-10-16 19:59:17 +0000
@@ -24,9 +24,9 @@
     return ''
 
 
-def set_captcha_answer(browser, answer=None):
+def set_captcha_answer(browser, answer=None, prefix=''):
     """Given a browser, set the login captcha with the correct answer."""
     if answer is None:
         answer = get_captcha_answer(browser.contents)
-    browser.getControl(name='loginpage_captcha_submission').value = (
+    browser.getControl(name=prefix + 'captcha_submission').value = (
         answer)