Merge pull request #140 from jagerman/subreq-exception-handling

Properly handle subrequest exceptions

Author: jagerman

sogs/model/exc.py

@@ -1,8 +1,3 @@
-from ..web import app
-from .. import http
-import flask
-
-
 class NotFound(LookupError):
     """Base class for NoSuchRoom, NoSuchFile, etc."""
 
@@ -84,24 +79,3 @@ class PostRateLimited(PostRejected):
 
     def __init__(self, msg=None):
         super().__init__("Rate limited" if msg is None else msg)
-
-
-# Map uncaught model exceptions into flask http exceptions
-@app.errorhandler(NotFound)
-def abort_bad_room(e):
-    flask.abort(http.NOT_FOUND)
-
-
-@app.errorhandler(BadPermission)
-def abort_perm_denied(e):
-    flask.abort(http.FORBIDDEN)
-
-
-@app.errorhandler(PostRejected)
-def abort_post_rejected(e):
-    flask.abort(http.TOO_MANY_REQUESTS)
-
-
-@app.errorhandler(InvalidData)
-def abort_invalid_data(e):
-    flask.abort(http.BAD_REQUEST)

sogs/routes/__init__.py

@@ -1,4 +1,5 @@
 from ..web import app
+
 from .legacy import legacy as legacy_endpoints
 from .general import general as general_endpoints
 from .onion_request import onion_request as onion_request_endpoints
@@ -8,6 +9,8 @@
 from .dm import dm as dm_endpoints
 from .views import views as views_endpoints
 
+from . import exc  # noqa: F401
+
 app.register_blueprint(dm_endpoints)
 app.register_blueprint(rooms_endpoints)
 app.register_blueprint(messages_endpoints)

sogs/routes/exc.py

@@ -0,0 +1,24 @@
+from ..web import app
+from .. import http
+from ..model import exc
+
+
+# Map uncaught model exceptions into flask http exceptions
+@app.errorhandler(exc.NotFound)
+def abort_bad_room(e):
+    return str(e), http.NOT_FOUND
+
+
+@app.errorhandler(exc.BadPermission)
+def abort_perm_denied(e):
+    return str(e), http.FORBIDDEN
+
+
+@app.errorhandler(exc.PostRejected)
+def abort_post_rejected(e):
+    return str(e), http.TOO_MANY_REQUESTS
+
+
+@app.errorhandler(exc.InvalidData)
+def abort_invalid_data(e):
+    return str(e), http.BAD_REQUEST

sogs/routes/subrequest.py

@@ -95,7 +95,10 @@ def make_subrequest(
         app.logger.debug(f"Initiating sub-request for {method} {path}")
         g.user_reauth = user_reauth
         with app.request_context(subreq_env):
-            response = app.full_dispatch_request()
+            try:
+                response = app.full_dispatch_request()
+            except Exception as e:
+                response = app.make_response(app.handle_exception(e))
         if response.status_code >= 400:
             app.logger.warning(
                 f"Sub-request for {method} {path} returned status {response.status_code}"

tests/test_room_routes.py

@@ -1,10 +1,8 @@
-import pytest
 import time
 from sogs.model.room import Room
 from sogs.model.file import File
 from sogs import utils, crypto
 import sogs.config
-import werkzeug.exceptions as wexc
 from util import pad64, from_now, config_override
 from auth import x_sogs
 from request import sogs_get, sogs_post, sogs_put, sogs_post_raw, sogs_delete
@@ -854,8 +852,8 @@ def room_json():
     assert room_json()['info_updates'] == 1
 
     url = "/room/test-room/pin/3"
-    with pytest.raises(wexc.Forbidden):
-        r = sogs_post(client, url, {}, user)
+    r = sogs_post(client, url, {}, user)
+    assert r.status_code == 403
 
     assert room_json()['info_updates'] == 1
 
@@ -965,8 +963,8 @@ def test_whisper_to(client, room, user, user2, mod, global_mod):
     p = {"data": d, "signature": s, "whisper_to": user2.session_id}
 
     # Regular users can't post whispers:
-    with pytest.raises(wexc.Forbidden):
-        r = sogs_post(client, url_post, p, user)
+    r = sogs_post(client, url_post, p, user)
+    assert r.status_code == 403
 
     r = sogs_post(client, url_post, p, mod)
     assert r.status_code == 201
@@ -1013,8 +1011,8 @@ def test_whisper_mods(client, room, user, user2, mod, global_mod, admin):
     p = {"data": d, "signature": s, "whisper_mods": True}
 
     # Regular users can't post mod whispers:
-    with pytest.raises(wexc.Forbidden):
-        r = sogs_post(client, url_post, p, user)
+    r = sogs_post(client, url_post, p, user)
+    assert r.status_code == 403
 
     r = sogs_post(client, url_post, p, mod)
     assert r.status_code == 201
@@ -1066,9 +1064,9 @@ def test_whisper_both(client, room, user, user2, mod, admin):
     }
 
     # Regular users can't post mod whispers:
-    with pytest.raises(wexc.Forbidden):
-        p = {"data": d, "signature": s, "whisper_mods": True, "whisper_to": mod.session_id}
-        r = sogs_post(client, url_post, p, user)
+    p = {"data": d, "signature": s, "whisper_mods": True, "whisper_to": mod.session_id}
+    r = sogs_post(client, url_post, p, user)
+    assert r.status_code == 403
 
     d, s = (utils.encode_base64(x) for x in (b"I'm going to scare this guy", pad64("sig2")))
     r = sogs_post(client, url_post, {"data": d, "signature": s, "whisper_mods": True}, mod)
@@ -1165,8 +1163,8 @@ def test_edits(client, room, user, user2, mod, global_admin):
 
     # Make sure someone else (even super admin) can't edit our message:
     d, s = (utils.encode_base64(x) for x in (b"post 1no", pad64("sig 1no")))
-    with pytest.raises(wexc.Forbidden):
-        r = sogs_put(client, url_edit, {"data": d, "signature": s}, global_admin)
+    r = sogs_put(client, url_edit, {"data": d, "signature": s}, global_admin)
+    assert r.status_code == 403
 
     r = sogs_get(client, url_get, user)
     assert filter_timestamps(r.json) == filter_timestamps([p1])
@@ -1260,8 +1258,8 @@ def test_remove_self_message(client, room, user):
 
 def test_remove_message_not_allowed(client, room, user, user2):
     id = _make_dummy_post(room, user)
-    with pytest.raises(wexc.Forbidden):
-        sogs_delete(client, f'/room/{room.token}/message/{id}', user2)
+    r = sogs_delete(client, f'/room/{room.token}/message/{id}', user2)
+    assert r.status_code == 403
 
 
 def test_remove_post_non_existing(client, room, user, mod):
@@ -1297,17 +1295,17 @@ def test_remove_all_posts_from_room_not_allowed(client, room, user, user2, no_ra
     for _ in range(256):
         _make_dummy_post(room, user)
     assert len(room.get_messages_for(user, recent=True)) == 256
-    with pytest.raises(wexc.Forbidden):
-        sogs_delete(client, f'/room/{room.token}/all/{user.session_id}', user2)
+    r = sogs_delete(client, f'/room/{room.token}/all/{user.session_id}', user2)
+    assert r.status_code == 403
     assert len(room.get_messages_for(user, recent=True)) == 256
     assert room.check_unbanned(user) and room.check_unbanned(user2)
 
 
 def test_remove_all_posts_from_room_not_allowed_for_user(client, room, mod, user, no_rate_limit):
     for _ in range(256):
         _make_dummy_post(room, mod)
-    with pytest.raises(wexc.Forbidden):
-        sogs_delete(client, f'/room/{room.token}/all/{mod.session_id}', user)
+    r = sogs_delete(client, f'/room/{room.token}/all/{mod.session_id}', user)
+    assert r.status_code == 403
     assert len(room.get_messages_for(user, recent=True)) == 256
     assert room.check_unbanned(user) and room.check_unbanned(mod)
 

tests/test_user_routes.py

@@ -1,6 +1,4 @@
-import pytest
 from sogs import db, utils
-import werkzeug.exceptions as wexc
 from request import sogs_get, sogs_post
 from util import pad64
 import time
@@ -553,8 +551,8 @@ def test_bans(client, room, room2, user, user2, mod, global_mod):
     r = sogs_post(client, url_ban, {'rooms': ['test-room']}, mod)
     assert r.status_code == 200
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
 
     r = sogs_post(client, "/room/test-room/message", post, user2)
     assert r.status_code == 201
@@ -570,11 +568,11 @@ def test_bans(client, room, room2, user, user2, mod, global_mod):
     r = sogs_post(client, url_ban, {'rooms': ['*']}, global_mod)
     assert r.status_code == 200
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/room2/message", post, user)
+    r = sogs_post(client, "/room/room2/message", post, user)
+    assert r.status_code == 403
 
     r = sogs_post(client, "/room/test-room/message", post, user2)
     assert r.status_code == 201
@@ -583,8 +581,8 @@ def test_bans(client, room, room2, user, user2, mod, global_mod):
 
     r = sogs_post(client, "/room/test-room/message", post, user)
     assert r.status_code == 201
-    with pytest.raises(wexc.Forbidden):
-        r = sogs_post(client, "/room/room2/message", post, user)
+    r = sogs_post(client, "/room/room2/message", post, user)
+    assert r.status_code == 403
 
     r = sogs_post(client, url_unban, {'rooms': ['*']}, global_mod)
 
@@ -641,11 +639,11 @@ def test_ban_timeouts(client, room, room2, user, mod, global_mod):
     r = sogs_post(client, url_ban, {'rooms': ['*'], 'timeout': 0.001}, global_mod)
     assert r.status_code == 200
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/room2/message", post, user)
+    r = sogs_post(client, "/room/room2/message", post, user)
+    assert r.status_code == 403
 
     from sogs.cleanup import cleanup
 
@@ -660,17 +658,18 @@ def test_ban_timeouts(client, room, room2, user, mod, global_mod):
     r = sogs_post(client, url_ban, {'rooms': ['*'], 'timeout': 30}, mod)
     assert r.status_code == 200
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
 
     r = sogs_post(client, "/room/room2/message", post, user)
     assert r.status_code == 201
 
     # The timed ban shouldn't expire yet:
     assert cleanup() == (0, 0, 0, 0, 0)
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
+
     r = sogs_post(client, "/room/room2/message", post, user)
     assert r.status_code == 201
 
@@ -683,8 +682,8 @@ def test_ban_timeouts(client, room, room2, user, mod, global_mod):
     r = sogs_post(client, url_ban, {'rooms': ['*'], 'timeout': 0.001}, mod)
     assert r.status_code == 200
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
 
     time.sleep(0.002)
     assert cleanup() == (0, 0, 0, 1, 0)
@@ -717,8 +716,8 @@ def test_ban_timeouts(client, room, room2, user, mod, global_mod):
 
     assert cleanup() == (0, 0, 0, 0, 0)
 
-    with pytest.raises(wexc.Forbidden):
-        sogs_post(client, "/room/test-room/message", post, user)
+    r = sogs_post(client, "/room/test-room/message", post, user)
+    assert r.status_code == 403
 
     # Unbanning should remove the ban future
     assert sogs_post(client, url_unban, {'rooms': ['*']}, mod).status_code == 200