Issue 2234 - Add a WSGI controller to collect email addresses for the Adblock Browser iOS launch

sitescripts/submitEmail/web/submitEmail.py

+# coding: utf-8
+
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2006-2015 Eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import errno
+import fcntl
+import random
+import string
+import wsgiref.util
+from urlparse import parse_qs, urljoin
+from urllib import urlencode
+
+from sitescripts.utils import get_config, sendMail
+from sitescripts.web import url_handler, form_handler
+
+VERIFICATION_PATH = '/verifyEmail'
+VERIFICATION_CODE_PARAM = 'code'

[kzar, 2015/04/21 17:14:31]

Nit: Seems to me that the constant VERIFICATION_CODE_PARAM is uglier than the
string 'code', and doesn't really add anything. It's only used twice, perhaps we
should just use the string directly instead?

[Sebastian Noack, 2015/04/21 18:10:30]

What's ugly about it? Yes, the code would be shorted without using a constant,
but code almost always is. However, the point of using a constant is to make it
more obvious that it must to be the same string in both places it is used, and
documenting it's purpose.

[kzar, 2015/04/22 09:17:46]

Well ugliness is subjective but to me VERIFICATION_CODE_PARAM looks a lot uglier
than "code".

+
+def generate_verification_code():
+  return ''.join(random.sample(string.letters + string.digits, 32))
+
+def get_filename_for_verification_code(config, verification_code):
+  directory = config.get('submitEmail', 'pendingVerficationsDirectory')
+  return os.path.join(directory, verification_code)
+
+def create_file_for_verification_code(config):
+  flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY | getattr(os, 'O_BINARY', 0)
+
+  while True:
+    verification_code = generate_verification_code()
+    filename = get_filename_for_verification_code(config, verification_code)
+
+    try:
+      return (os.open(filename, flags), verification_code)
+    except OSError as e:
+      if e.errno != errno.EEXIST:
+        raise
+
+def verify_email_address(config, verification_code):
+  if verification_code in ('', os.path.curdir, os.path.pardir):

[kzar, 2015/04/21 17:14:31]

Maybe we should check that the code matches a regexp of exactly what we're
expecting? (A 32 character alpha-numeric string.)

[Sebastian Noack, 2015/04/21 18:10:30]

This would duplicate the format of the verification code. And even then,
omitting the logic here would imply that neither dots nor dashes can occur in
the verification code, wile this assumption is currently true, code assumptions
are always foot guns waiting to triggered by future changes.

[kzar, 2015/04/22 09:17:46]

Well if the filterhits code review is anything to go by Wladimir will agree with
me here. I'm not saying you should remove the other checks, just that you should
probably check that it's a 32 character alpha-numeric string as well.

[Sebastian Noack, 2015/04/22 10:06:00]

I don't know which comment in that other review you are referring to. But I
can't remember any case where we encouraged duplication of logic.

It is arguable whether following check would be simpler than the logic currently
used here, and therefore preferable despite the points I outlined above:

  if not re.search('^[a-z0-9]+$', verification_code, re.I):
    return False

But introducing redundant logic is certainly not what we are used to do.

[kzar, 2015/04/22 10:13:04]

Exactly, that's what I meant, I think we should add that check.

[Sebastian Noack, 2015/04/22 12:19:06]

Ok, went with this now, but without duplicating the list of characters. 

+    return False
+  if os.path.sep in verification_code:
+    return False
+
+  verification_filename = get_filename_for_verification_code(config, verification_code)
+  try:
+    file = open(verification_filename, 'rb')
+  except IOError as e:
+    if e.errno == errno.ENOENT:
+      return False
+    raise
+
+  with file:
+    email = file.read()
+
+  verified_filename = config.get('submitEmail', 'verifiedEmailAddressesFile')
+  with open(verified_filename, 'ab', 0) as file:
+    fcntl.lockf(file, fcntl.LOCK_EX)
+    try:
+      print >>file, email
+    finally:
+      fcntl.lockf(file, fcntl.LOCK_UN)
+
+  try:
+    os.unlink(verification_filename)
+  except OSError as e:
+    if e.errno != errno.ENOENT:
+      raise
+
+  return True
+
+@url_handler('/submitEmail')
+@form_handler
+def submitEmail(environ, start_response, data):
+  email = data.get('email', '').strip()
+  if not email or '\r' in email or '\n' in email:

[kzar, 2015/04/21 17:14:31]

Should we also check that the email address hasn't already been submitted and
verified?

[Sebastian Noack, 2015/04/21 18:10:30]

That won't scale the way data are stored. But we can simply remove duplicates
before sending the notification email later.

[kzar, 2015/04/22 09:17:46]

OK, fair enough.

+    start_response('400 Bad Request', [('Content-Type', 'text/plain')])
+    if email:
+      message = 'No newlines allowed in email address.'
+    else
+      message = 'No email address given.'
+    return [message]

[kzar, 2015/04/21 17:14:31]

Nit: Instead of assigning `message` wouldn't it be easier to just return the
message string wrapped in a list directly?

[Sebastian Noack, 2015/04/21 18:10:30]

Then the else block would be redundant. And removing it would introduce
inconsistent code structure. So I found it best to use a variable here.

[kzar, 2015/04/22 09:17:46]

Well it just seems like busy-work to me. Keeping something redundant in the code
in order to avoid something else becoming redundant seems like weird logic.

(Also it looks like the else clause is missing the trailing colon.)

[Sebastian Noack, 2015/04/22 10:06:00]

Wouldn't it be the same when doing it the other way around? However, it doesn't
make much a difference which poison to pick here. So done now.
This has already been fixed (you are looking on an outdated version of the
patch).

+
+  config = get_config()
+  fd, verification_code = create_file_for_verification_code(config)
+  try:
+    os.write(fd, email.encode('utf-8'))
+  finally:
+    os.close(fd)
+
+  sendMail(
+    config.get('submitEmail', 'verificationEmailTemplate'),
+    {
+      'recipient': email,
+      'verification_url': '%s?%s' % (
+        urljoin(wsgiref.util.application_uri(environ), VERIFICATION_PATH),
+        urlencode([(VERIFICATION_CODE_PARAM, verification_code)])
+      )
+    }
+  )
+
+  start_response('200 OK', [('Content-Type', 'text/plain')])
+  return ["Thanks for your submission! You'll receive a verification email shortly."]
+
+@url_handler(VERIFICATION_PATH)
+def verifyEmail(environ, start_response):
+  config = get_config()
+
+  params = parse_qs(environ.get('QUERY_STRING', ''))
+  verification_code = params.get(VERIFICATION_CODE_PARAM, [''])[0]
+
+  if verify_email_address(config, verification_code):
+    option = 'successfulVerificationRedirectLocation'
+  else:
+    option = 'unknownVerificationCodeRedirectLocation'
+
+  start_response('303 See other', [('Location', config.get('submitEmail', option))])
+  return []