From 94de0d59e49aadb99c9882087e55d157dd22c9e8 Mon Sep 17 00:00:00 2001
From: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>
Date: Tue, 26 Feb 2019 16:59:00 +0100
Subject: [PATCH] Add json response handler for server internal errors

Fixes #3035

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>
(cherry picked from commit 2d309869752ee04143e7af043d76220c3e1269c2)
---
 bodhi/server/static/js/forms.js              | 32 +++++++++----
 bodhi/server/templates/master.html           | 22 +++++++++
 bodhi/server/views/generic.py                | 48 +++++++++++++++++---
 bodhi/tests/server/functional/test_admin.py  |  3 +-
 bodhi/tests/server/functional/test_stacks.py |  3 +-
 bodhi/tests/server/views/test_generic.py     | 45 ++++++++++++++----
 6 files changed, 128 insertions(+), 25 deletions(-)

diff --git a/bodhi/server/static/js/forms.js b/bodhi/server/static/js/forms.js
index 84126f3177..cec3e66eec 100644
--- a/bodhi/server/static/js/forms.js
+++ b/bodhi/server/static/js/forms.js
@@ -70,15 +70,31 @@ Form.prototype.success = function(data) {
 Form.prototype.error = function(data) {
     var self = this;
     self.finish();
-    // Here is where we handle those error messages on the response by cornice.
-    $.each(data.responseJSON.errors, function (i, error) {
-        msg = self.messenger.post({
-            message: error.name.capitalize() + ' : ' + error.description,
-            type: "error",
-            hideAfter: false,
-            showCloseButton: true,
+    if (data.status >= 500) {
+        // In case of Internal Server Error show error modal
+        var msg = '';
+
+        $.each(data.responseJSON.errors, function (i, error) {
+            if (msg != '') {
+                msg += '; ';
+            }
+            msg += error.name.capitalize() + ' : ' + error.description;
         });
-    });
+
+        $('#alertModalDescription').text(msg);
+        $('#alertModal').modal('show');
+    }
+    else {
+        // Here is where we handle those error messages on the response by cornice.
+        $.each(data.responseJSON.errors, function (i, error) {
+            msg = self.messenger.post({
+                message: error.name.capitalize() + ' : ' + error.description,
+                type: "error",
+                hideAfter: false,
+                showCloseButton: true,
+            });
+        });
+    }
 }
 
 Form.prototype.data = function() {
diff --git a/bodhi/server/templates/master.html b/bodhi/server/templates/master.html
index c029f6d193..33a05454af 100644
--- a/bodhi/server/templates/master.html
+++ b/bodhi/server/templates/master.html
@@ -224,6 +224,28 @@
       </div>
     </div>
 
+    <!-- Alert Modal -->
+    <div class="modal fade" id="alertModal" tabindex="-1" role="dialog" aria-labelledby="alertModalLabel" aria-hidden="true" data-backdrop="static" data-keyboard="false">
+      <div class="modal-dialog" role="document">
+	<div class="modal-content">
+          <div class="modal-header">
+            <h5 class="modal-title" id="alertModalLabel">Internal Server Error!</h5>
+          </div>
+          <div class="modal-body" id="alertModalDescription">
+	     The server encountered an internal error
+          </div>
+          <div class="modal-footer">
+            <button type="button" class="btn btn-secondary" data-dismiss="modal">Close</button>
+          </div>
+        </div>
+      </div>
+    </div>
+    <script>
+      $('#alertModal').on('hidden.bs.modal', function (e) {
+          document.location.href = document.baseURI;
+      })
+    </script>
+
     <!-- Placed at the end of the document so the pages load faster -->
     <script src="${request.static_url('bodhi:server/static/jquery.flot.js')}"></script>
     <script src="${request.static_url('bodhi:server/static/jquery.flot.stack.js')}"></script>
diff --git a/bodhi/server/views/generic.py b/bodhi/server/views/generic.py
index 458004d9a8..88e07312a8 100644
--- a/bodhi/server/views/generic.py
+++ b/bodhi/server/views/generic.py
@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# Copyright © 2014-2017 Red Hat, Inc. and others
+# Copyright © 2014-2019 Red Hat, Inc. and others
 #
 # This file is part of Bodhi.
 #
@@ -428,14 +428,14 @@ def notfound_view(context, request):
         bodhi.server.services.errors.html_handler: A pyramid.httpexceptions.HTTPError to be rendered
             to the user for the 404.
     """
-    return exception_view(context, request)
+    return exception_html_view(context, request)
 
 
-@view_config(context=HTTPForbidden)
-@view_config(context=Exception)
-def exception_view(exc, request):
+@view_config(context=HTTPForbidden, accept='text/html')
+@view_config(context=Exception, accept='text/html')
+def exception_html_view(exc, request):
     """
-    Return an error response upon generic errors (404s, 403s, 500s, etc..).
+    Return a html error response upon generic errors (404s, 403s, 500s, etc..).
 
     This is here to catch everything that isn't caught by our cornice error
     handlers.  When we do catch something, we transform it into a cornice
@@ -464,3 +464,39 @@ def exception_view(exc, request):
         errors.add('body', description=description)
 
     return bodhi.server.services.errors.html_handler(errors, request)
+
+
+@view_config(context=HTTPForbidden, accept='application/json')
+@view_config(context=Exception, accept='application/json')
+def exception_json_view(exc, request):
+    """
+    Return a json error response upon generic errors (404s, 403s, 500s, etc..).
+
+    This is here to catch everything that isn't caught by our cornice error
+    handlers.  When we do catch something, we transform it into a cornice
+    Errors object and pass it to our nice cornice error handler.  That way, all
+    the exception presentation and rendering we can keep in one place.
+
+    Args:
+        exc (Exception): The unhandled exception.
+        request (pyramid.util.Request): The current request.
+    Returns:
+        bodhi.server.services.errors.html_handler: A pyramid.httpexceptions.HTTPError to be rendered
+            to the user for the given exception.
+    """
+    errors = getattr(request, 'errors', [])
+    status = getattr(exc, 'status_code', 500)
+
+    if status not in (404, 403):
+        log.exception("Error caught.  Handling JSON response.")
+    else:
+        log.warning(str(exc))
+
+    if not len(errors):
+        description = getattr(exc, 'explanation', None) or str(exc)
+
+        errors = cornice.errors.Errors(status=status)
+        errors.add('body', description=description, name=exc.__class__.__name__)
+        request.errors = errors
+
+    return bodhi.server.services.errors.json_handler(request)
diff --git a/bodhi/tests/server/functional/test_admin.py b/bodhi/tests/server/functional/test_admin.py
index 29a07f28e3..47e6ae91e4 100644
--- a/bodhi/tests/server/functional/test_admin.py
+++ b/bodhi/tests/server/functional/test_admin.py
@@ -37,12 +37,13 @@ def test_admin(self):
 
     def test_admin_unauthed(self):
         """Test that an unauthed user cannot see the admin endpoint"""
+        headers = {'Accept': 'text/html'}
         anonymous_settings = copy.copy(self.app_settings)
         anonymous_settings.update({
             'authtkt.secret': 'whatever',
             'authtkt.secure': True,
         })
         app = webtest.TestApp(main({}, session=self.db, **anonymous_settings))
-        res = app.get('/admin/', status=403)
+        res = app.get('/admin/', status=403, headers=headers)
         self.assertIn('<h1>403 <small>Forbidden</small></h1>', res)
         self.assertIn('<p class="lead">Access was denied to this resource.</p>', res)
diff --git a/bodhi/tests/server/functional/test_stacks.py b/bodhi/tests/server/functional/test_stacks.py
index 29e4c00f2f..cc669453dc 100644
--- a/bodhi/tests/server/functional/test_stacks.py
+++ b/bodhi/tests/server/functional/test_stacks.py
@@ -277,12 +277,13 @@ def test_new_stack_form_unauthed(self):
         """
         Assert we get a 403 if the user is not logged in
         """
+        headers = {'Accept': 'text/html'}
         anonymous_settings = copy.copy(self.app_settings)
         anonymous_settings.update({
             'authtkt.secret': 'whatever',
             'authtkt.secure': True,
         })
         app = webtest.TestApp(main({}, session=self.db, **anonymous_settings))
-        res = app.get('/stacks/new', status=403)
+        res = app.get('/stacks/new', status=403, headers=headers)
         self.assertIn('<h1>403 <small>Forbidden</small></h1>', res)
         self.assertIn('<p class="lead">Access was denied to this resource.</p>', res)
diff --git a/bodhi/tests/server/views/test_generic.py b/bodhi/tests/server/views/test_generic.py
index 07d2a3e599..f372f8455f 100644
--- a/bodhi/tests/server/views/test_generic.py
+++ b/bodhi/tests/server/views/test_generic.py
@@ -36,14 +36,34 @@ class TestExceptionView(base.BaseTestCase):
     @mock.patch('bodhi.server.views.generic.log.exception')
     @mock.patch('bodhi.server.views.metrics.compute_ticks_and_data',
                 mock.MagicMock(side_effect=RuntimeError("BOOM")))
-    def test_status_500(self, exception):
+    def test_status_500_html(self, exception):
         """Assert that a status 500 code causes the exception to get logged."""
-        res = self.app.get('/metrics', status=500)
+        headers = {'Accept': 'text/html'}
+        res = self.app.get('/metrics', status=500, headers=headers)
 
+        self.assertEqual('text/html', res.content_type)
         self.assertIn('Server Error', res)
         self.assertIn('BOOM', res)
         exception.assert_called_once_with("Error caught.  Handling HTML response.")
 
+    @mock.patch('bodhi.server.views.generic.log.exception')
+    @mock.patch('bodhi.server.views.metrics.compute_ticks_and_data',
+                mock.MagicMock(side_effect=RuntimeError("BOOM")))
+    def test_status_500_json(self, exception):
+        """Assert that a status 500 code causes the exception to get logged."""
+        headers = {'Content-Type': 'application/json'}
+        res = self.app.get('/metrics', status=500, headers=headers)
+
+        self.assertEqual('application/json', res.content_type)
+        self.assertEqual(
+            res.json_body,
+            {
+                "status": "error",
+                "errors": [{"location": "body", "name": "RuntimeError", "description": "BOOM"}]
+            },
+        )
+        exception.assert_called_once_with("Error caught.  Handling JSON response.")
+
 
 class TestGenericViews(base.BaseTestCase):
 
@@ -348,15 +368,18 @@ def test_masher_status(self):
 
     def test_popup_toggle(self):
         """Check that the toggling of pop-up notifications works"""
+
+        headers = {'Accept': 'text/html'}
+
         # first we check that popups are enabled by default
-        res = self.app.get('/')
+        res = self.app.get('/', headers=headers)
         self.assertIn('Disable popups', res)
 
         # toggle popups off
         self.app.post('/popup_toggle')
 
         # now check popups are off
-        res = self.app.get('/')
+        res = self.app.get('/', headers=headers)
         self.assertIn('Enable popups', res)
 
         # test that the unlogged in user cannot toggle popups
@@ -366,15 +389,17 @@ def test_popup_toggle(self):
             'authtkt.secure': True,
         })
         app = webtest.TestApp(main({}, session=self.db, **anonymous_settings))
-        res = app.post('/popup_toggle', status=403)
+        res = app.post('/popup_toggle', status=403, headers=headers)
         self.assertIn('<h1>403 <small>Forbidden</small></h1>', res)
         self.assertIn('<p class="lead">Access was denied to this resource.</p>', res)
 
     def test_new_override_form(self):
         """Test the New Override form page"""
 
+        headers = {'Accept': 'text/html'}
+
         # Test that the New Override form shows when logged in
-        res = self.app.get('/overrides/new')
+        res = self.app.get('/overrides/new', headers=headers)
         self.assertIn('<span>New Buildroot Override Form Requires JavaScript</span>', res)
 
         # Test that the unlogged in user cannot see the New Override form
@@ -384,15 +409,17 @@ def test_new_override_form(self):
             'authtkt.secure': True,
         })
         app = webtest.TestApp(main({}, session=self.db, **anonymous_settings))
-        res = app.get('/overrides/new', status=403)
+        res = app.get('/overrides/new', status=403, headers=headers)
         self.assertIn('<h1>403 <small>Forbidden</small></h1>', res)
         self.assertIn('<p class="lead">Access was denied to this resource.</p>', res)
 
     def test_new_update_form(self):
         """Test the new update Form page"""
 
+        headers = {'Accept': 'text/html'}
+
         # Test that a logged in user sees the New Update form
-        res = self.app.get('/updates/new')
+        res = self.app.get('/updates/new', headers=headers)
         self.assertIn('Creating a new update requires JavaScript', res)
         # Make sure that unspecified comes first, as it should be the default.
         regex = r''
@@ -407,7 +434,7 @@ def test_new_update_form(self):
             'authtkt.secure': True,
         })
         app = webtest.TestApp(main({}, session=self.db, **anonymous_settings))
-        res = app.get('/updates/new', status=403)
+        res = app.get('/updates/new', status=403, headers=headers)
         self.assertIn('<h1>403 <small>Forbidden</small></h1>', res)
         self.assertIn('<p class="lead">Access was denied to this resource.</p>', res)