Merge pull request #11784 from archesproject/jtw/custom-error-handlers

Restore custom error views (but return JSON where desired) #11722

arches/app/templates/errors/400.htm

@@ -0,0 +1,29 @@
+{% extends "errors/error_base.htm" %}
+{% load i18n %}
+{% load static %}
+
+{% block body %}
+<div style="text-align: center">
+	<!--===================================================-->
+	<div class="cls-header">
+	    <div class="cls-brand">
+	        <a class="box-inline" href="/index.html">
+	            <span class="brand-title"><h4>{% trans "Arches" %}</h4></span>
+	        </a>
+	    </div>
+	</div>
+
+	<!-- CONTENT -->
+	<!--===================================================-->
+	<div class="cls-content">
+	    <h1 class="error-code text-primary">{% trans "Oops!" %}</h1>
+	    <p class="h3 text-uppercase">{% trans "Bad Request" %}</p>
+	    <div class="pad-btm">
+	        {% trans "Something went wrong and we couldn't process your request." %}"
+	    </div>
+
+	    <hr class="new-section-sm bord-no">
+	    <div class="pad-top"><a class="btn-link" href="/index.html">{% trans "Back to Homepage" %}</a></div>
+	</div>
+</div>
+{% endblock body %}

arches/app/templates/errors/403.htm

@@ -0,0 +1,29 @@
+{% extends "errors/error_base.htm" %}
+{% load i18n %}
+{% load static %}
+
+{% block body %}
+<div style="text-align: center">
+	<!--===================================================-->
+	<div class="cls-header">
+	    <div class="cls-brand">
+	        <a class="box-inline" href="/index.html">
+	            <span class="brand-title"><h4>{% trans "Arches" %}</h4></span>
+	        </a>
+	    </div>
+	</div>
+
+	<!-- CONTENT -->
+	<!--===================================================-->
+	<div class="cls-content">
+	    <h1 class="error-code text-primary">{% trans "Oops!" %}</h1>
+	    <p class="h3 text-uppercase">{% trans "Access Denied" %}</p>
+	    <div class="pad-btm">
+	        {% trans "You don't have permission to access this resource." %}"
+	    </div>
+
+	    <hr class="new-section-sm bord-no">
+	    <div class="pad-top"><a class="btn-link" href="/index.html">{% trans "Back to Homepage" %}</a></div>
+	</div>
+</div>
+{% endblock body %}

arches/app/templates/errors/404.htm

@@ -19,11 +19,11 @@
 	    <h1 class="error-code text-mint">{% trans "Oops!" %}</h1>
 	    <p class="h3 text-uppercase">{% trans "Page Not Found!" %}</p>
 	    <div class="pad-btm">
-				{% trans "Sorry, but the page you are looking for has not been found on our server." %}
+			{% trans "Sorry, but the page you are looking for has not been found on our server." %}
 	    </div>
 
 	    <hr class="new-section-sm bord-no">
 	    <div class="pad-top"><a class="btn-link" href="/index.html">{% trans "Back to Homepage" %}</a></div>
 	</div>
 </div>
-{% endblock body %}
+{% endblock body %}

arches/app/templates/errors/500.htm

@@ -15,7 +15,7 @@
 
 	<!-- CONTENT -->
 	<!--===================================================-->
-	<div class="cls-content" >
+	<div class="cls-content">
 	    <h1 class="error-code text-primary">{% trans "Oops!" %}</h1>
 	    <p class="h3 text-uppercase">{% trans "Internal Server Error!" %}</p>
 	    <div class="pad-btm">

arches/app/utils/request.py

@@ -0,0 +1,17 @@
+def looks_like_api_call(request):
+    known_api_paths_without_api_signposting = [
+        # Until these are renamed for consistency, just list them.
+        "arches.app.views.workflow_history",
+    ]
+    if "/api/" in request.path:
+        return True
+    if not request.resolver_match:
+        return False
+    if ".api." in request.resolver_match.func.__module__:
+        return True
+    if (
+        request.resolver_match.func.__module__
+        in known_api_paths_without_api_signposting
+    ):
+        return True
+    return False

arches/app/views/main.py

@@ -16,14 +16,18 @@
 along with this program. If not, see <http://www.gnu.org/licenses/>.
 """
 
-import re
 import urllib.request, urllib.error, urllib.parse
+from http import HTTPStatus
 from urllib.parse import urlparse
+
+from django.http import HttpResponseNotFound, HttpResponse
+from django.shortcuts import render
+from django.utils.translation import gettext as _
+
 from arches import __version__
 from arches.app.models.system_settings import settings
-from django.shortcuts import render, redirect
-from django.http import HttpResponseNotFound, HttpResponse, HttpResponseRedirect
-from django.utils import translation
+from arches.app.utils.request import looks_like_api_call
+from arches.app.utils.response import JSONErrorResponse
 
 
 def index(request):
@@ -88,11 +92,36 @@ def feature_popup_content(request):
         return HttpResponseNotFound()
 
 
-def custom_404(request):
-    request = None
-    return render(request, "errors/404.htm")
+def custom_400(request, exception=None):
+    status = HTTPStatus.BAD_REQUEST
+    if looks_like_api_call(request):
+        return JSONErrorResponse(_("Request Failed"), _("Bad Request"), status=status)
+    return render(request, "errors/400.htm", status=status)
+
+
+def custom_403(request, exception=None):
+    status = HTTPStatus.FORBIDDEN
+    if looks_like_api_call(request):
+        return JSONErrorResponse(
+            _("Request Failed"), _("Permission Denied"), status=status
+        )
+    return render(request, "errors/403.htm", status=status)
+
+
+def custom_404(request, exception=None):
+    status = HTTPStatus.NOT_FOUND
+    if looks_like_api_call(request):
+        return JSONErrorResponse(_("Request Failed"), _("Not Found"), status=status)
+    return render(request, "errors/404.htm", status=status)
 
 
-def custom_500(request):
-    request = None
-    return render(request, "errors/500.htm")
+def custom_500(request, exception=None):
+    status = HTTPStatus.INTERNAL_SERVER_ERROR
+    if looks_like_api_call(request):
+        return JSONErrorResponse(
+            # Exclamation point matches translated string in template.
+            _("Request Failed"),
+            _("Internal Server Error!"),
+            status=status,
+        )
+    return render(request, "errors/500.htm", status=status)

arches/install/arches-templates/project_name/urls.py-tpl

@@ -7,6 +7,11 @@ urlpatterns = [
     # project-level urls
 ]
 
+handler400 = "arches.app.views.main.custom_400"
+handler403 = "arches.app.views.main.custom_403"
+handler404 = "arches.app.views.main.custom_404"
+handler500 = "arches.app.views.main.custom_500"
+
 # Ensure Arches core urls are superseded by project-level urls
 urlpatterns.append(path('', include('arches.urls')))
 

arches/urls.py

@@ -781,9 +781,13 @@
         api.SpatialView.as_view(),
         name="spatialview_api",
     ),
-    re_path("^api", api.API404.as_view(), name="api_404"),
 ]
 
+handler400 = "arches.app.views.main.custom_400"
+handler403 = "arches.app.views.main.custom_403"
+handler404 = "arches.app.views.main.custom_404"
+handler500 = "arches.app.views.main.custom_500"
+
 # This must be included in core to keep webpack happy, but cannot be appended when running a project.
 # See https://github.com/archesproject/arches/pull/10754
 if settings.ROOT_URLCONF == __name__:

releases/8.0.0.md

@@ -31,6 +31,7 @@ Arches 8.0.0 Release Notes
 - Add system dark mode detection for Vue apps [#11624](https://github.com/archesproject/arches/issues/11624)
 - Make number datatype node values searchable in the main search [#11619](https://github.com/archesproject/arches/issues/11619)
 - Prevent navigation to a new browser tab when clicking Manage link in index.htm [#11635](https://github.com/archesproject/arches/issues/11635)
+- Mitigate the tendency of JSON APIs to respond with HTML under error conditions [#11722](https://github.com/archesproject/arches/pull/11722)
 - Add support for tile sort order to the bulk data manager [#11638](https://github.com/archesproject/arches/pull/11638)
 - Fix issue that produced an error when cycling between the dropdown options in Advanced search [#11442](https://github.com/archesproject/arches/issues/11442)
 - Adds message when copying geojson url to clipboard [#11076](https://github.com/archesproject/arches/issues/11076)
@@ -132,6 +133,15 @@ JavaScript:
     python manage.py updateproject
     ```
 
+1. Update your urls.py to include generic error handlers:
+
+    ```
+    handler400 = "arches.app.views.main.custom_400"
+    handler403 = "arches.app.views.main.custom_403"
+    handler404 = "arches.app.views.main.custom_404"
+    handler500 = "arches.app.views.main.custom_500"
+    ```
+
 3. Create editable_future_graphs for your Resource Models using the command `python manage.py graph create_editable_future_graphs`. This will publish new versions of each Graph.
 
 4. Update your Graph publications and Resource instances to point to the newly published Graphs by running `python manage.py graph publish --update -ui`

tests/views/api/test_resources.py

@@ -26,7 +26,7 @@
 from django.urls import reverse
 from django.contrib.auth.models import User
 from django.test.client import RequestFactory
-from django.test.utils import captured_stdout
+from django.test.utils import captured_stdout, override_settings
 from unittest.mock import patch, MagicMock
 
 from arches.app.views.api import APIBase
@@ -120,11 +120,14 @@ def test_api_base_view(self):
             response = view(request)
         self.assertEqual(request.GET.get("ver"), "2.1")
 
-    def test_api_404(self):
+    @override_settings(DEBUG=False)
+    def test_api_404_returns_json(self):
         with self.assertLogs("django.request", level="WARNING"):
-            response = self.client.get(reverse("api_404"))
-        self.assertEqual(
-            set(json.loads(response.content)), {"message", "status", "success", "title"}
+            response = self.client.get("/api/doesnotexist")
+        self.assertContains(
+            response,
+            "Not Found",
+            status_code=HTTPStatus.NOT_FOUND,
         )
 
     def test_api_resources_archesjson(self):

tests/views/workflow_tests.py

@@ -1,5 +1,6 @@
 import uuid
 import datetime
+from http import HTTPStatus
 
 from django.contrib.auth.models import Group, User
 from django.urls import reverse
@@ -83,8 +84,9 @@ def test_get_workflow_history(self):
                 )
             )
 
-        self.assertEqual(response.status_code, 403)
-        self.assertIn(b"Forbidden", response.content)
+        self.assertContains(
+            response, "Permission Denied", status_code=HTTPStatus.FORBIDDEN
+        )
 
         self.client.force_login(self.admin)
         response = self.client.get(
@@ -93,8 +95,7 @@ def test_get_workflow_history(self):
             )
         )
 
-        self.assertEqual(response.status_code, 200)
-        self.assertIn(b"sample name", response.content)
+        self.assertContains(response, "sample name", status_code=HTTPStatus.OK)
 
     def test_post_workflow_history(self):
         """Partial updates of componentdata and stepdata are allowed."""
@@ -148,8 +149,9 @@ def test_post_workflow_history(self):
                 content_type="application/json",
             )
 
-        self.assertEqual(response.status_code, 403)
-        self.assertIn(b"Forbidden", response.content)
+        self.assertContains(
+            response, "Permission Denied", status_code=HTTPStatus.FORBIDDEN
+        )
 
         self.client.force_login(self.admin)
         response = self.client.post(