Issue 4377 - Add Configurable Form to Email Service

sitescripts/formmail/web/formmail2.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 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 re
 import datetime
 import collections
 from urlparse import parse_qsl
 from sitescripts.utils import get_config, sendMail, setupStderr

[Sebastian Noack, 2016/09/12 22:11:49]

There should be a blank line between corelib and own module imports.

[Vasily Kuznetsov, 2016/09/13 09:03:28]

Done.

 from sitescripts.web import registerUrlHandler
 
 
 class BadRequestError(Exception):
     pass
 
 
 class ConfDict(collections.OrderedDict):
     __slots__ = ('value',)

[Sebastian Noack, 2016/09/12 22:11:50]

This micro optimization doesn't do anything, since the base class, i.e.
OrderedDict, requires instances to have a __dict__, anyway. The only thing
__slots__ does, is skipping the creation of a per instance attributes
dictionary, if possible. The attributes listed will be the only attributes that
can be accessed on those instances then. Otherwise, any name can be accessed as
attribute. However, if you derive from a class that requires a __dict__ for its
instances, i.e. any non-builtin class that doesn't define __slots__, it doesn't
have any effect to set __slots__ in the derived class.

FWIW, it seems cleaner anyway, to just pass the dictionary and that value with a
tuple, rather than storing the latter as an attribute of the former.

[Vasily Kuznetsov, 2016/09/13 07:15:53]

Yeah, with OrderedDict __slots__ is pretty much useless. Originally this code
was inheriting from dict, so it made more sense.

Since we're building a tree of dictionaries here, the approach with tuples would
make navigating the whole data structure more complicated, but we could just
remove ConfDict class, and replace it with OrderedDict in the rest of the code.

[Vasily Kuznetsov, 2016/09/13 09:03:27]

Done.

 
 
 def get_config_items():
     config = get_config()
     default_keys = set(config.defaults())
     for name, value in config.items('formmail2'):
         if name not in default_keys:
             yield name, value
 
 
@@ skipping 10 matching lines @@
 def conf_parse(conf_items):
     conf_dict = ConfDict()
     for key, value in conf_items:
         path = key.split('.')
         store_value(conf_dict, path, value)
     return conf_dict
 
 
 def post_handler(handler):
     def wrapped_handler(environ, start_response):
         setupStderr(environ['wsgi.errors'])

[Sebastian Noack, 2016/09/12 22:11:49]

setupStderr() is depreacted. You should actually see a deprecation warning when
running that code from a terminal.

[Vasily Kuznetsov, 2016/09/13 07:15:53]

Yeah, I saw the warning. Since we're not writing anything to `stderr` I suppose
we could just delete this line.

[Vasily Kuznetsov, 2016/09/13 09:03:27]

Done.

         response_headers = [('Content-Type', 'text/plain; charset=utf-8')]
 
         try:
             request_method = environ['REQUEST_METHOD'].upper()
             url_encoded = 'application/x-www-form-urlencoded'
             is_url_encoded = environ.get(
                     'CONTENT_TYPE', '').startswith(url_encoded)
             if request_method != 'POST' or not is_url_encoded:
                 raise BadRequestError('Unsupported request method')
             try:
                 request_body_length = int(environ['CONTENT_LENGTH'])

[Sebastian Noack, 2016/09/12 22:11:49]

If CONTENT_LENGTH, is not given but required, 411 rather than 400, should be
returned. Also note that the logic here is mostly redundant with the
sitescripts.web.form_handler() decorator.

[Vasily Kuznetsov, 2016/09/13 07:15:52]

Yes, that decorator does pretty much everything that the `post_handler` here
does except for converting exceptions to error responses. I think the best
solution would be to delete lines 60-78 here, add `params` as the last argument
of `wrapped_handler` and then apply `form_handler` decorator to it.

[Vasily Kuznetsov, 2016/09/13 09:03:27]

In the end after removing all `form_handler` logic it didn't seem like having
this decorator made sense. I got rid of it and of the BadRequestError exception.

             except:
                 raise BadRequestError(
                         'Invalid or missing Content-Length header')
             request_body = environ['wsgi.input'].read(request_body_length)
             params = {}
             for key, value in parse_qsl(request_body):
                 params[key] = value.decode('utf-8').strip()
 
             response = handler(params)
         except BadRequestError as error:
-            start_response('400 Bad Request'+str(error), response_headers)

[Vasily Kuznetsov, 2016/09/12 11:33:20]

There should be spaces around that '+' (PEP8). Dunno why flake8 doesn't pick it
up...

+            start_response('400 Bad Request', response_headers)
             return str(error)
         start_response('200 OK', response_headers)
         return response
     return wrapped_handler
+
+
+def make_error(spec, check_type, default_message):
+    if check_type in spec:
+        return spec[check_type].value
+    return default_message
 
 
 def make_handler(name, config):
     try:
         url = config['url'].value
     except (KeyError, AttributeError):
         raise Exception('No URL configured for form handler:' + name)
     try:
         template = config['template'].value
     except (KeyError, AttributeError):
         raise Exception('No template configured for form handler:' + name)
     try:
         fields = config['fields']
         for field, spec in fields.items():
             spec.value = set(spec.value.replace(' ', '').split(','))

[Sebastian Noack, 2016/09/12 22:11:49]

It seems you want to ignore optional spaces around commas here. However, if you
eliminate spaces separately before you slit the string, this will turn 'foo bar,
baz' into ['foobar', 'baz'] which most likely isn't intended. It seems using a
regexp is more appropriate here:

  re.split(r'\s*,\s*', spec.value)

[Vasily Kuznetsov, 2016/09/13 07:15:53]

Perhaps regexes are a bit overkill here. How about:

spec.value = set(s.strip() for s in spec.value.split(','))

[Vasily Kuznetsov, 2016/09/13 09:03:27]

Done.

     except KeyError:
         raise Exception('No fields configured for form handler:' + name)
     if len(fields) == 0:
         raise Exception('No fields configured for form handler:' + name)
 
     @post_handler
     def handler(params):
         email_regex = r'^\w[\w.+!-]+@\w[\w.-]+\.[a-zA-Z]{2,6}$'

[Sebastian Noack, 2016/09/12 22:11:50]

There is utils.encode_email_address(), which we use across sitescripts, in
particular in new code, in order to validate email addresses. So we should use
it here as well, rather than implementing yet another validation method.

FWIW, the regexp here has some assumptions that will not necessarily always be
true, yet there are other ways to construct an invalid email address that it
won't catch. Correctly validating email addresses is hard till impossible, but
fortunately all we need to check for is that there an @-sign, no white space,
and only ASCII characters. Every mail server, client or API can safely handle
addresses in that form, and will just return an error, as it might do anyway, if
there is still something wrong with the address. encode_email_address() does
exactly that, and also handles IDN domains.

[Vasily Kuznetsov, 2016/09/13 07:15:52]

Yeah, this regex was just copied from formmail.py. Using encode_email_address
makes more sense.

[Vasily Kuznetsov, 2016/09/13 09:03:27]

Done.

         errors = []
         for field, spec in fields.items():
             if 'mandatory' in spec.value:
-                if field not in params.keys() or not params[field]:
-                    if 'mandatory' in spec:
-                        errors.append(spec['mandatory'].value)
-                    else:
-                        errors.append('No {} entered'.format(field))
-            if 'email' in spec.value and 'email' in params.keys():
-                if not re.search(email_regex, params['email']):
-                    if 'mandatory' in spec:

[Vasily Kuznetsov, 2016/09/12 11:33:20]

This should be `if 'email' in spec:`, etc.

Also, this code duplication is not great. Perhaps we could use a function like
this to abstract this error message logic:

def make_error(field_name, spec, check_type, error_message):
    if check_type in spec:
        return spec[check_type].value
    return error_message.format(field_name)

-                        errors.append(spec['mandatory'].value)
-                    else:
-                        errors.append('Invalid email address')
+                if field not in params.keys():
+                    errors.append(make_error(spec, 'mandatory',
+                                             'No {} entered'.format(field)))
+            if 'email' in spec.value and field in params.keys():
+                if not re.search(email_regex, params[field]):
+                    errors.append(make_error(spec, 'email',
+                                             'Invalid email'))
         if errors:
             raise BadRequestError('\n'.join(errors))
-
-        params['time'] = datetime.datetime.now()
-        sendMail(template, {'fields': params})
+        template_args = {
+            'time': datetime.datetime.now(),
+            'fields': {field: params.get(field, '') for field in fields}
+        }
+        sendMail(template, template_args)
         return ''
     return url, handler
-
 
 conf_dict = conf_parse(get_config_items())
 for name, config in conf_dict.items():
     url, handler = make_handler(name, config)
     registerUrlHandler(url, handler)