replace abort(code) with proper exceptions and refactor request dispatch

isso/__init__.py

@@ -32,7 +32,6 @@ dist = pkg_resources.get_distribution("isso")
 
 import sys
 import os
-import socket
 import logging
 
 from os.path import dirname, join
@@ -47,8 +46,7 @@ import misaka
 from itsdangerous import URLSafeTimedSerializer
 
 from werkzeug.routing import Map, Rule
-from werkzeug.wrappers import Response, Request
+from werkzeug.exceptions import HTTPException, InternalServerError
-from werkzeug.exceptions import HTTPException, NotFound, MethodNotAllowed
 
 from werkzeug.wsgi import SharedDataMiddleware
 from werkzeug.serving import run_simple
@@ -56,7 +54,7 @@ from werkzeug.contrib.fixers import ProxyFix
 
 from isso import db, migrate, views, wsgi
 from isso.core import ThreadedMixin, uWSGIMixin, Config
-from isso.utils import parse, http
+from isso.utils import parse, http, JSONRequest
 from isso.views import comment
 
 logging.getLogger('werkzeug').setLevel(logging.ERROR)
@@ -67,25 +65,23 @@ logging.basicConfig(
 logger = logging.getLogger("isso")
 
 
-rules = Map([
-    Rule('/new', methods=['POST'], endpoint=views.comment.new),
-
-    Rule('/id/<int:id>', methods=['GET', 'PUT', 'DELETE'], endpoint=views.comment.single),
-    Rule('/id/<int:id>/like', methods=['POST'], endpoint=views.comment.like),
-    Rule('/id/<int:id>/dislike', methods=['POST'], endpoint=views.comment.dislike),
-
-    Rule('/', methods=['GET'], endpoint=views.comment.fetch),
-    Rule('/count', methods=['GET'], endpoint=views.comment.count),
-    Rule('/delete/<string:auth>', endpoint=views.comment.delete),
-    Rule('/activate/<string:auth>', endpoint=views.comment.activate),
-
-    Rule('/check-ip', endpoint=views.comment.checkip)
-])
-
-
 class Isso(object):
 
     salt = b"Eech7co8Ohloopo9Ol6baimi"
+    urls = Map([
+        Rule('/new', methods=['POST'], endpoint=views.comment.new),
+
+        Rule('/id/<int:id>', methods=['GET', 'PUT', 'DELETE'], endpoint=views.comment.single),
+        Rule('/id/<int:id>/like', methods=['POST'], endpoint=views.comment.like),
+        Rule('/id/<int:id>/dislike', methods=['POST'], endpoint=views.comment.dislike),
+
+        Rule('/', methods=['GET'], endpoint=views.comment.fetch),
+        Rule('/count', methods=['GET'], endpoint=views.comment.count),
+        Rule('/delete/<string:auth>', endpoint=views.comment.delete),
+        Rule('/activate/<string:auth>', endpoint=views.comment.activate),
+
+        Rule('/check-ip', endpoint=views.comment.checkip)
+    ])
 
     def __init__(self, conf):
 
@@ -102,30 +98,27 @@ class Isso(object):
         return self.signer.loads(obj, max_age=max_age or self.conf.getint('general', 'max-age'))
 
     def markdown(self, text):
-        return misaka.html(text, extensions=misaka.EXT_STRIKETHROUGH \
+        return misaka.html(text, extensions=misaka.EXT_STRIKETHROUGH
-            | misaka.EXT_SUPERSCRIPT | misaka.EXT_AUTOLINK \
+            | misaka.EXT_SUPERSCRIPT | misaka.EXT_AUTOLINK
             | misaka.HTML_SKIP_HTML  | misaka.HTML_SKIP_IMAGES | misaka.HTML_SAFELINK)
 
-    def dispatch(self, request, start_response):
+    def dispatch(self, request):
-        adapter = rules.bind_to_environ(request.environ)
+        adapter = Isso.urls.bind_to_environ(request.environ)
         try:
             handler, values = adapter.match()
-            return handler(self, request.environ, request, **values)
-        except NotFound:
-            return Response('Not Found', 404)
-        except MethodNotAllowed:
-            return Response("Yup.", 200)
         except HTTPException as e:
             return e
+        else:
+            try:
+                response = handler(self, request.environ, request, **values)
+            except HTTPException as e:
+                return e
+            except Exception:
+                logger.exception("%s %s", request.method, request.environ["PATH_INFO"])
+                return InternalServerError()
 
-    def wsgi_app(self, environ, start_response):
-
-        response = self.dispatch(Request(environ), start_response)
-
-        # add CORS header
-        if hasattr(response, 'headers') and 'HTTP_ORIGN' in environ:
             for host in self.conf.getiter('general', 'host'):
-                if environ["HTTP_ORIGIN"] == host.rstrip("/"):
+                if request.environ.get("HTTP_ORIGIN", None) == host.rstrip("/"):
                     origin = host.rstrip("/")
                     break
             else:
@@ -137,6 +130,11 @@ class Isso(object):
             hdrs["Access-Control-Allow-Credentials"] = "true"
             hdrs["Access-Control-Allow-Methods"] = "GET, POST, PUT, DELETE"
 
+            return response
+
+    def wsgi_app(self, environ, start_response):
+
+        response = self.dispatch(JSONRequest(environ))
         return response(environ, start_response)
 
     def __call__(self, environ, start_response):

isso/utils/__init__.py

@@ -2,11 +2,15 @@
 
 from __future__ import division
 
+import json
 import random
 import hashlib
 
 from string import ascii_letters, digits
 
+from werkzeug.wrappers import Request
+from werkzeug.exceptions import BadRequest
+
 import ipaddress
 
 
@@ -83,3 +87,12 @@ class Bloomfilter:
 
     def __len__(self):
         return self.elements
+
+
+class JSONRequest(Request):
+
+    def get_json(self):
+        try:
+            return json.loads(self.get_data().decode('utf-8'))
+        except ValueError:
+            raise BadRequest('Unable to read JSON request')

isso/views/comment.py

@@ -10,7 +10,7 @@ import sqlite3
 from itsdangerous import SignatureExpired, BadSignature
 
 from werkzeug.wrappers import Response
-from werkzeug.exceptions import abort, BadRequest
+from werkzeug.exceptions import BadRequest, Forbidden, NotFound
 
 from isso.compat import text_type as str
 
@@ -49,19 +49,16 @@ class requires:
 @requires(str, 'uri')
 def new(app, environ, request, uri):
 
-    try:
+    data = request.get_json()
-        data = json.loads(request.get_data().decode('utf-8'))
-    except ValueError:
-        return Response("No JSON object could be decoded", 400)
 
     for field in set(data.keys()) - set(['text', 'author', 'website', 'email', 'parent']):
         data.pop(field)
 
     if not data.get("text"):
-        return Response("No text given.", 400)
+        raise BadRequest("no text given")
 
     if "id" in data and not isinstance(data["id"], int):
-        return Response("Parent ID must be an integer.")
+        raise BadRequest("parent id must be an integer")
 
     for field in ("author", "email"):
         if data.get(field):
@@ -88,11 +85,8 @@ def new(app, environ, request, uri):
     try:
         with app.lock:
             rv = app.db.comments.add(uri, data)
-    except sqlite3.Error:
-        logging.exception('uncaught SQLite3 exception')
-        abort(400)
     except db.IssoDBException:
-        abort(403)
+        raise Forbidden
 
     host = list(app.conf.getiter('general', 'host'))[0].rstrip("/")
     href = host + uri + "#isso-%i" % rv["id"]
@@ -130,7 +124,7 @@ def single(app, environ, request, id):
     if request.method == 'GET':
         rv = app.db.comments.get(id)
         if rv is None:
-            abort(404)
+            raise NotFound
 
         for key in set(rv.keys()) - FIELDS:
             rv.pop(key)
@@ -146,23 +140,20 @@ def single(app, environ, request, id):
         try:
             rv = app.unsign(request.cookies.get('admin', ''))
         except (SignatureExpired, BadSignature):
-            abort(403)
+            raise Forbidden
 
     if rv[0] != id:
-        abort(403)
+        raise Forbidden
 
     # verify checksum, mallory might skip cookie deletion when he deletes a comment
     if rv[1] != hashlib.md5(app.db.comments.get(id)["text"].encode('utf-8')).hexdigest():
-        abort(403)
+        raise Forbidden
 
     if request.method == 'PUT':
-        try:
+        data = request.get_json()
-            data = json.loads(request.get_data().decode('utf-8'))
-        except ValueError:
-            return Response("No JSON object could be decoded", 400)
 
         if data.get("text") is not None and len(data['text']) < 3:
-            return Response("No text given.", 400)
+            raise BadRequest("no text given")
 
         for key in set(data.keys()) - set(["text", "author", "website"]):
             data.pop(key)
@@ -203,7 +194,7 @@ def fetch(app, environ, request, uri):
 
     rv = list(app.db.comments.fetch(uri))
     if not rv:
-        abort(404)
+        raise NotFound
 
     for item in rv:
 
@@ -237,7 +228,7 @@ def count(app, environ, request, uri):
     rv = app.db.comments.count(uri)[0]
 
     if rv == 0:
-        abort(404)
+        raise NotFound
 
     return Response(json.dumps(rv), 200, content_type='application/json')
 
@@ -247,7 +238,7 @@ def activate(app, environ, request, auth):
     try:
         id = app.unsign(auth, max_age=2**32)
     except (BadSignature, SignatureExpired):
-        abort(403)
+        raise Forbidden
 
     with app.lock:
         app.db.comments.activate(id)
@@ -260,7 +251,7 @@ def delete(app, environ, request, auth):
     try:
         id = app.unsign(auth, max_age=2**32)
     except (BadSignature, SignatureExpired):
-        abort(403)
+        raise Forbidden
 
     with app.lock:
         app.db.comments.delete(id)