Przeglądaj źródła

Skip UserConfig saves in maintenance mode, and give the maintenance mode page its own template

Brian Tiemann 8 godzin temu
rodzic
commit
2c433413a6

+ 2 - 2
netbox/extras/views.py

@@ -1114,7 +1114,7 @@ class ConfigContextView(generic.ObjectView):
         if request.GET.get('format') in ['json', 'yaml']:
             format = request.GET.get('format')
             if request.user.is_authenticated:
-                request.user.config.set('data_format', format, commit=not get_config().MAINTENANCE_MODE)
+                request.user.config.set('data_format', format, commit=True)
         elif request.user.is_authenticated:
             format = request.user.config.get('data_format', 'json')
         else:
@@ -1176,7 +1176,7 @@ class ObjectConfigContextView(generic.ObjectView):
         if request.GET.get('format') in ['json', 'yaml']:
             format = request.GET.get('format')
             if request.user.is_authenticated:
-                request.user.config.set('data_format', format, commit=not get_config().MAINTENANCE_MODE)
+                request.user.config.set('data_format', format, commit=True)
         elif request.user.is_authenticated:
             format = request.user.config.get('data_format', 'json')
         else:

+ 3 - 2
netbox/netbox/middleware.py

@@ -8,8 +8,9 @@ from django.core.exceptions import ImproperlyConfigured
 from django.core.signals import got_request_exception
 from django.db import Error, ProgrammingError, connection
 from django.db.utils import InternalError
-from django.http import Http404, HttpResponseRedirect
+from django.http import Http404, HttpResponse, HttpResponseRedirect
 from django.middleware.common import CommonMiddleware as DjangoCommonMiddleware
+from django.template import loader
 from django.utils.translation import gettext_lazy as _
 from django_prometheus import middleware
 from social_django.middleware import SocialAuthExceptionMiddleware as SocialAuthExceptionMiddleware_
@@ -296,7 +297,7 @@ class MaintenanceModeMiddleware:
 
             # Redirecting a read-only request (e.g. GET) back to the same URL would fail the same way again
             if request.method in ('GET', 'HEAD', 'OPTIONS'):
-                return handler_500(request, template_name='exceptions/maintenance_mode.html')
+                return HttpResponse(loader.render_to_string('exceptions/maintenance_mode.html'), status=503)
 
             messages.error(request, error_message)
             return HttpResponseRedirect(request.path_info)

+ 2 - 5
netbox/netbox/tables/tables.py

@@ -17,7 +17,6 @@ from django_tables2.utils import OrderBy
 from core.models import ObjectType
 from extras.choices import *
 from extras.models import CustomField, CustomLink
-from netbox.config import get_config
 from netbox.constants import EMPTY_TABLE_TEXT
 from netbox.registry import registry
 from netbox.tables import columns
@@ -228,16 +227,14 @@ class BaseTable(tables.Table):
         ordering = None
 
         if request.user.is_authenticated and self.prefixed_order_by_field in request.GET:
-            # Don't save the preference while the database is read-only (maintenance mode)
-            commit = not get_config().MAINTENANCE_MODE
             if request.GET[self.prefixed_order_by_field]:
                 # If an ordering has been specified as a query parameter, save it as the
                 # user's preferred ordering for this table.
                 ordering = request.GET.getlist(self.prefixed_order_by_field)
-                request.user.config.set(f'tables.{self.name}.ordering', ordering, commit=commit)
+                request.user.config.set(f'tables.{self.name}.ordering', ordering, commit=True)
             else:
                 # If the ordering has been set to none (empty), clear any existing preference.
-                request.user.config.clear(f'tables.{self.name}.ordering', commit=commit)
+                request.user.config.clear(f'tables.{self.name}.ordering', commit=True)
 
         # If the user has a saved preference, apply it
         if request.user.is_authenticated and (userconfig := request.user.config):

+ 21 - 1
netbox/netbox/tests/test_middleware.py

@@ -169,8 +169,9 @@ class CoreMiddlewareTestCase(TestCase):
         with patch('netbox.middleware.get_config', return_value=SimpleNamespace(MAINTENANCE_MODE=True)):
             response = self.process_internal_error(request, 'Simulated maintenance mode UI error')
 
-        self.assertEqual(response.status_code, 500)
+        self.assertEqual(response.status_code, 503)
         self.assertIn('maintenance mode', response.content.decode())
+        self.assertNotIn('Server Error', response.content.decode())
 
     def test_maintenance_mode_handles_graphql_json_request(self):
         request = self.factory.post(
@@ -280,6 +281,25 @@ class MaintenanceModeTestCase(TransactionTestCase):
         self.assertIn('maintenance mode', response.json()['error'])
         self.assertFalse(Site.objects.exists())
 
+    def test_api_user_config_write_refused(self):
+        """
+        Explicitly saving a user's preferences is refused, unlike the incidental changes made while viewing a page.
+        """
+        token = Token.objects.create(user=self.user)
+        header = {'HTTP_AUTHORIZATION': f'Bearer {TOKEN_PREFIX}{token.key}.{token.token}'}
+
+        response = self.client.patch(
+            reverse('users-api:userconfig-list'),
+            {'data_format': 'yaml'},
+            content_type='application/json',
+            **header,
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)
+        self.assertIn('maintenance mode', response.json()['error'])
+        self.user.config.refresh_from_db()
+        self.assertIsNone(self.user.config.get('data_format'))
+
     def test_writes_resume_when_maintenance_mode_disabled(self):
         self.client.force_login(self.user)
         data = {'name': 'Site 1', 'slug': 'site-1', 'status': 'active'}

+ 1 - 4
netbox/netbox/views/generic/mixins.py

@@ -1,7 +1,6 @@
 from django.shortcuts import get_object_or_404
 
 from extras.models import TableConfig
-from netbox.config import get_config
 from utilities.permissions import get_permission_for_model
 
 __all__ = (
@@ -57,9 +56,7 @@ class TableMixin:
             if request.user.is_authenticated:
                 table = self.table.__name__
                 request.user.config.set(f'tables.{table}.columns', tableconfig.columns)
-                request.user.config.set(
-                    f'tables.{table}.ordering', tableconfig.ordering, commit=not get_config().MAINTENANCE_MODE
-                )
+                request.user.config.set(f'tables.{table}.ordering', tableconfig.ordering, commit=True)
 
         table = self.table(data)
         if 'pk' in table.base_columns and bulk_actions:

+ 2 - 1
netbox/templates/500.html

@@ -5,6 +5,7 @@
 
 <head>
   <title>{% trans "Server Error" %}</title>
+  <link rel="stylesheet" href="{% static 'netbox-external.css' %}" />
   <link rel="stylesheet" href="{% static 'netbox.css'%}" />
   <meta charset="UTF-8">
 </head>
@@ -15,7 +16,7 @@
       <div class="col col-md-6 offset-md-3">
         <div class="card border-danger mt-5">
           <h2 class="card-header text-bg-danger">
-            <i class="mdi mdi-alert"></i> {% trans "Server Error" %}
+            <i class="mdi mdi-alert me-1"></i> {% trans "Server Error" %}
           </h2>
           <div class="card-body">
             {% block message %}

+ 33 - 6
netbox/templates/exceptions/maintenance_mode.html

@@ -1,8 +1,35 @@
-{% extends '500.html' %}
 {% load i18n %}
+{% load static %}
+<!DOCTYPE html>
+<html lang="en">
 
-{% block message %}
-  <p>
-    {% trans "NetBox is currently operating in maintenance mode and is unable to perform write operations. Please try again later." %}
-  </p>
-{% endblock message %}
+<head>
+  <title>{% trans "Maintenance Mode" %}</title>
+  <link rel="stylesheet" href="{% static 'netbox-external.css' %}" />
+  <link rel="stylesheet" href="{% static 'netbox.css'%}" />
+  <meta charset="UTF-8">
+</head>
+
+<body>
+  <div class="container-fluid">
+    <div class="row">
+      <div class="col col-md-6 offset-md-3">
+        <div class="card border-warning mt-5">
+          <h2 class="card-header text-bg-warning">
+            <i class="mdi mdi-wrench me-1"></i> {% trans "Maintenance Mode" %}
+          </h2>
+          <div class="card-body">
+            <p>
+              {% trans "NetBox is currently operating in maintenance mode and is unable to perform write operations. Please try again later." %}
+            </p>
+            <div class="text-end">
+              <a href="{% url 'home' %}" class="btn btn-primary">{% trans "Home Page" %}</a>
+            </div>
+          </div>
+        </div>
+      </div>
+    </div>
+  </div>
+</body>
+
+</html>

+ 12 - 4
netbox/users/models/preferences.py

@@ -81,7 +81,8 @@ class UserConfig(models.Model):
 
         :param path: Dotted path to the configuration key. For example, 'foo.bar' sets self.data['foo']['bar'].
         :param value: The value to be written. This can be any type supported by JSON.
-        :param commit: If true, the UserConfig instance will be saved once the new value has been applied.
+        :param commit: If true, the UserConfig instance will be saved once the new value has been applied. (The save
+            is skipped while maintenance mode is enabled, since the database may be read-only.)
         """
         d = self.data
         keys = path.split('.')
@@ -112,7 +113,7 @@ class UserConfig(models.Model):
             d[key] = value
 
         if commit:
-            self.save()
+            self._commit()
     set.alters_data = True
 
     def clear(self, path, commit=False):
@@ -125,7 +126,8 @@ class UserConfig(models.Model):
         Invalid keys will be ignored silently.
 
         :param path: Dotted path to the configuration key. For example, 'foo.bar' deletes self.data['foo']['bar'].
-        :param commit: If true, the UserConfig instance will be saved once the new value has been applied.
+        :param commit: If true, the UserConfig instance will be saved once the new value has been applied. (The save
+            is skipped while maintenance mode is enabled, since the database may be read-only.)
         """
         d = self.data
         keys = path.split('.')
@@ -140,5 +142,11 @@ class UserConfig(models.Model):
         d.pop(key, None)  # Avoid a KeyError on invalid keys
 
         if commit:
-            self.save()
+            self._commit()
     clear.alters_data = True
+
+    def _commit(self):
+        # Don't save the change while maintenance mode is enabled: the database may be read-only, and these changes
+        # are typically incidental to a read-only request (e.g. a table's ordering).
+        if not get_config().MAINTENANCE_MODE:
+            self.save()

+ 14 - 0
netbox/users/tests/test_models.py

@@ -233,6 +233,20 @@ class UserConfigTestCase(TestCase):
         # Clear a non-existing value; should fail silently
         userconfig.clear('invalid')
 
+    @override_settings(MAINTENANCE_MODE=True)
+    def test_commit_skipped_in_maintenance_mode(self):
+        userconfig = User.objects.get(username='testuser').config
+
+        # The change is applied, but not saved
+        userconfig.set('a', 'def', commit=True)
+        userconfig.clear('b.foo', commit=True)
+        self.assertEqual(userconfig.data['a'], 'def')
+        self.assertNotIn('foo', userconfig.data['b'])
+
+        userconfig.refresh_from_db()
+        self.assertEqual(userconfig.data['a'], True)
+        self.assertEqual(userconfig.data['b']['foo'], 101)
+
 
 class RestrictedQuerySetTestCase(TestCase):
     """