Fixes #2479 - Refactors the form to be more testable

tests/unit/test_form.py

@@ -10,6 +10,7 @@
 
 import webcompat
 from webcompat import form
+from webcompat import helpers
 
 FIREFOX_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0'  # nopep8
 
@@ -188,12 +189,46 @@ def test_get_details(self):
         self.assertEqual(actual_json_arg, expected_json_arg)
 
     def test_build_details(self):
-        """Assert we return the expected HTML, for a json object or a string.
-        """
+        """Assert we return the expected HTML, for a json object or a string."""
         actual_json_arg = form.build_details(json.dumps(
             {'a': 'b', 'c': False}))
         expected_json_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n  <li>a: b</li><li>c: false</li>\n</ul>\n</details>'  # nopep8
         self.assertEqual(actual_json_arg, expected_json_arg)
         actual_string_arg = form.build_details("cool")
         expected_string_arg = '<details>\n<summary>Browser Configuration</summary>\n<ul>\n  cool\n</ul>\n</details>'  # nopep8
         self.assertEqual(actual_string_arg, expected_string_arg)
+
+    def test_is_valid_issue_form(self):
+        """Assert that we get the form parameters we want."""
+        incomplete_form = MultiDict([('problem_category', u'unknown_bug')])
+        self.assertFalse(helpers.is_valid_issue_form(incomplete_form))
+        valid_form = MultiDict([
+            ('browser', u'Firefox 61.0'),
+            ('description', u'streamlining the form.'),
+            ('details', u''),
+            ('os', u'Mac OS X 10.13'),
+            ('problem_category', u'unknown_bug'),
+            ('submit_type', u'github-auth-report'),
+            ('url', u'http://2479.example.com'),
+            ('username', u''),
+            ])
+        self.assertTrue(helpers.is_valid_issue_form(valid_form))
+        # The value for submit-Type can be only:
+        # - github-auth-report
+        # - github-proxy-report
+        wrong_value_form = MultiDict([
+            ('browser', u'Firefox 61.0'),
+            ('description', u'streamlining the form.'),
+            ('details', u''),
+            ('os', u'Mac OS X 10.13'),
+            ('problem_category', u'unknown_bug'),
+            ('submit_type', u'wrong-value'),
+            ('url', u'http://2479.example.com'),
+            ('username', u''),
+            ])
+        self.assertFalse(helpers.is_valid_issue_form(wrong_value_form))
+
+    def test_is_blacklisted_domain(self):
+        """Assert domains validity in issue reporting."""
+        self.assertTrue(helpers.is_blacklisted_domain('coco.fr'))
+        self.assertFalse(helpers.is_blacklisted_domain('w3.org'))

webcompat/helpers.py

@@ -552,3 +552,35 @@ def get_milestone_list(milestone_name, params=None):
         milestone_name, other_params=params)
     milestone_list = json.loads(raw_response[0])
     return milestone_list
+
+
+def is_valid_issue_form(form):
+    """Check if the issue form follows some requirements.
+
+    To be legit the form needs a couple of parameters
+    if one essential is missing, it's a bad request.
+    We may add more constraints in the future.
+    """
+    must_parameters = [
+        'browser',
+        'description',
+        'os',
+        'problem_category',
+        'submit_type',
+        'url',
+        'username',
+        ]
+    form_submit_values = ['github-auth-report', 'github-proxy-report']
+    parameters_check = set(must_parameters).issubset(form.keys())
+    if parameters_check:
+        values_check = form['submit_type'] in form_submit_values
+    return parameters_check and values_check
+
+
+def is_blacklisted_domain(domain):
+    """Check if the domain is part of an exclusion list."""
+    # see https://github.com/webcompat/webcompat.com/issues/1141
+    # see https://github.com/webcompat/webcompat.com/issues/1237
+    # see https://github.com/webcompat/webcompat.com/issues/1627
+    spamlist = ['qiangpiaoruanjian', 'cityweb.de', 'coco.fr']
+    return domain in spamlist

webcompat/views.py

@@ -28,6 +28,8 @@
 from helpers import get_milestone_list
 from helpers import get_referer
 from helpers import get_user_info
+from helpers import is_blacklisted_domain
+from helpers import is_valid_issue_form
 from helpers import set_referer
 from issues import report_issue
 from webcompat import app
@@ -169,9 +171,16 @@ def show_issues():
 def create_issue():
     """Create a new issue.
 
-    GET will return an HTML response for reporting issues
-    POST will create a new issue
+    GET will return an HTML response for reporting issues.
+    POST will create a new issue.
+
+    Any deceptive requests will be ended as a 400.
+    See https://tools.ietf.org/html/rfc7231#section-6.5.1
     """
+    # Starting a logger
+    log = app.logger
+    log.setLevel(logging.INFO)
+    # GET Requests
     if request.method == 'GET':
         bug_form = get_form(request.headers.get('User-Agent'))
         if g.user:
@@ -186,40 +195,32 @@ def create_issue():
         if request.args.get('label'):
             session['label'] = request.args.getlist('label')
         return render_template('new-issue.html', form=bug_form)
-    # copy the form so we can add the full UA string to it.
+    # POST Requests
     if request.form:
+        # Copy the form to add the full UA string.
         form = request.form.copy()
-        # To be legit the form needs a couple of parameters
-        # if one essential is missing, it's a bad request
-        must_parameters = set(['url', 'problem_category', 'description',
-                               'os', 'browser',
-                               'username', 'submit_type'])
-        if not must_parameters.issubset(form.keys()):
+        if not is_valid_issue_form(form):
             abort(400)
     else:
-        # https://tools.ietf.org/html/rfc7231#section-6.5.1
+        log.info('POST request without form.')
         abort(400)
-    # see https://github.com/webcompat/webcompat.com/issues/1141
-    # see https://github.com/webcompat/webcompat.com/issues/1237
-    # see https://github.com/webcompat/webcompat.com/issues/1627
-    spamlist = ['qiangpiaoruanjian', 'cityweb.de', 'coco.fr']
-    for spam in spamlist:
-        if spam in form.get('url'):
-            msg = (u'Anonymous reporting for domain {0} '
-                   'is temporarily disabled. Please contact '
-                   '[email protected] '
-                   'for more details.').format(spam)
-            flash(msg, 'notimeout')
-            return redirect(url_for('index'))
+    # Logging the ip and url for investigation
+    log.info('{ip} {url}'.format(
+        ip=request.remote_addr,
+        url=form['url'].encode('utf-8'))
+        )
+    # Checking blacklisted domains
+    if is_blacklisted_domain(form['url']):
+        msg = (u'Anonymous reporting for domain {0} '
+               'is temporarily disabled. Please contact '
+               '[email protected] '
+               'for more details.').format(form['url'])
+        flash(msg, 'notimeout')
+        return redirect(url_for('index'))
     form['ua_header'] = request.headers.get('User-Agent')
     form['reported_with'] = session.pop('src', 'web')
     # Reminder: label is a list, if it exists
     form['extra_labels'] = session.pop('label', None)
-    # Logging the ip and url for investigation
-    log = app.logger
-    log.setLevel(logging.INFO)
-    log.info('{ip} {url}'.format(ip=request.remote_addr,
-                                 url=form['url'].encode('utf-8')))
     # form submission for 3 scenarios: authed, to be authed, anonymous
     if form.get('submit_type') == AUTH_REPORT:
         if g.user:  # If you're already authed, submit the bug.