View authentication and permission fixes (#5464)

changes/5464.added

@@ -0,0 +1,2 @@
+Added `nautobot.apps.utils.get_url_for_url_pattern` and `nautobot.apps.utils.get_url_patterns` lookup functions.
+Added `nautobot.apps.views.GenericView` base class.

changes/5464.changed

@@ -0,0 +1,2 @@
+Added support for `view_name` and `view_description` optional parameters when instantiating a `nautobot.apps.api.OrderedDefaultRouter`. Specifying these parameters is to be preferred over defining a custom `APIRootView` subclass when defining App API URLs.
+Added requirement for user authentication by default on the `nautobot.apps.api.APIRootView` class. As a consequence, viewing the browsable REST API root endpoints (e.g. `/api/`, `/api/circuits/`, `/api/dcim/`, etc.) now requires user authentication.

changes/5464.documentation

@@ -0,0 +1 @@
+Updated example views in the App developer documentation to include `ObjectPermissionRequiredMixin` or `LoginRequiredMixin` as appropriate best practices.

changes/5464.fixed

@@ -0,0 +1 @@
+Fixed a 500 error when accessing any of the `/dcim/<port-type>/<uuid>/connect/<termination_b_type>/` view endpoints with an invalid/nonexistent `termination_b_type` string.

changes/5464.housekeeping

@@ -0,0 +1 @@
+Updated custom views in the `example_plugin` to use the new `GenericView` base class as a best practice.

changes/5464.removed

@@ -0,0 +1 @@
+Removed the URL endpoints `/api/users/users/my-profile/`, `/api/users/users/session/`, `/api/users/tokens/authenticate/`, and `/api/users/tokens/logout/` as they are unused at this time.

changes/5464.security

@@ -0,0 +1,8 @@
+Added requirement for user authentication to access the endpoint `/extras/job-results/<uuid:pk>/log-table/`; furthermore it will not allow an authenticated user to view log entries for a JobResult they don't otherwise have permission to view. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added narrower permissions enforcement on the endpoints `/extras/git-repositories/<uuid:pk>/sync/` and `/extras/git-repositories/<uuid:pk>/dry-run/`; a user who has `change` permissions for a subset of Git repositories is no longer permitted to sync or dry-run other repositories for which they lack the appropriate permissions. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added narrower permissions enforcement on the `/api/dcim/connected-device/?peer_device=...&?peer_interface=...` REST API endpoint; a user who has `view` permissions for a subset of interfaces is no longer permitted to query other interfaces for which they lack permissions. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added narrower permissions enforcement on all `<app>/<model>/<uuid>/notes/` UI endpoints; a user must now have the appropriate `extras.view_note` permissions to view existing notes. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added requirement for user authentication to access the REST API endpoints `/api/redoc/`, `/api/swagger/`, `/api/swagger.json`, and `/api/swagger.yaml`. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added requirement for user authentication to access the `/api/graphql` REST API endpoint, even when `EXEMPT_VIEW_PERMISSIONS` is configured. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added requirement for user authentication to access the endpoints `/dcim/racks/<uuid>/dynamic-groups/`, `/dcim/devices/<uuid>/dynamic-groups/`, `/ipam/prefixes/<uuid>/dynamic-groups/`, `/ipam/ip-addresses/<uuid>/dynamic-groups/`, `/virtualization/clusters/<uuid>/dynamic-groups/`, and `/virtualization/virtual-machines/<uuid>/dynamic-groups/`, even when `EXEMPT_VIEW_PERMISSIONS` is configured. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))
+Added requirement for user authentication to access the endpoint `/extras/secrets/provider/<str:provider_slug>/form/`. ([GHSA-m732-wvh2-7cq4](https://github.com/nautobot/nautobot/security/advisories/GHSA-m732-wvh2-7cq4))

examples/example_plugin/example_plugin/api/urls.py

@@ -4,7 +4,7 @@
 
 from example_plugin.api.views import AnotherExampleModelViewSet, ExampleModelViewSet, ExampleModelWebhook
 
-router = OrderedDefaultRouter()
+router = OrderedDefaultRouter(view_name="Example App")
 router.register("models", ExampleModelViewSet)
 router.register("other-models", AnotherExampleModelViewSet)
 

examples/example_plugin/example_plugin/views.py

@@ -1,5 +1,4 @@
 from django.shortcuts import HttpResponse, render
-from django.views.generic import View
 from rest_framework.decorators import action
 
 from nautobot.apps import views
@@ -46,12 +45,12 @@ class DeviceDetailPluginTabTwoView(views.ObjectView):
     template_name = "example_plugin/tab_device_detail_2.html"
 
 
-class ExamplePluginHomeView(View):
+class ExamplePluginHomeView(views.GenericView):
     def get(self, request):
         return render(request, "example_plugin/home.html")
 
 
-class ExamplePluginConfigView(View):
+class ExamplePluginConfigView(views.GenericView):
     def get(self, request):
         """Render the configuration page for this plugin.
 
@@ -114,6 +113,6 @@ class AnotherExampleModelUIViewSet(
     table_class = tables.AnotherExampleModelTable
 
 
-class ViewToBeOverridden(View):
+class ViewToBeOverridden(views.GenericView):
     def get(self, request, *args, **kwargs):
         return HttpResponse("I am a view in the example plugin which will be overridden by another plugin.")

examples/example_plugin_with_view_override/example_plugin_with_view_override/views.py

@@ -1,10 +1,11 @@
 """Views for plugin_with_view_override."""
 
 from django.shortcuts import HttpResponse
-from django.views import generic
 
+from nautobot.apps.views import GenericView
 
-class ViewOverride(generic.View):
+
+class ViewOverride(GenericView):
     def get(self, request, *args, **kwargs):
         return HttpResponse("Hello world! I'm an overridden view.")
 

nautobot/apps/api.py

@@ -19,7 +19,7 @@
 )
 from nautobot.core.api.mixins import WritableSerializerMixin
 from nautobot.core.api.parsers import NautobotCSVParser
-from nautobot.core.api.routers import OrderedDefaultRouter
+from nautobot.core.api.routers import AuthenticatedAPIRootView as APIRootView, OrderedDefaultRouter
 from nautobot.core.api.schema import NautobotAutoSchema
 from nautobot.core.api.serializers import (
     OptInFieldsMixin,
@@ -36,7 +36,6 @@
     versioned_serializer_selector,
 )
 from nautobot.core.api.views import (
-    APIRootView,
     BulkDestroyModelMixin,
     BulkUpdateModelMixin,
     GetObjectCountsView,

nautobot/apps/utils.py

@@ -32,6 +32,8 @@
     get_related_class_for_model,
     get_route_for_model,
     get_table_for_model,
+    get_url_for_url_pattern,
+    get_url_patterns,
 )
 from nautobot.core.utils.navigation import (
     get_all_new_ui_ready_routes,
@@ -110,6 +112,8 @@
     "get_route_for_model",
     "get_settings_or_config",
     "get_table_for_model",
+    "get_url_for_url_pattern",
+    "get_url_patterns",
     "get_worker_count",
     "GitRepo",
     "hex_to_rgb",

nautobot/apps/views.py

@@ -8,6 +8,7 @@
     BulkImportView,
     BulkRenameView,
     ComponentCreateView,
+    GenericView,
     ObjectDeleteView,
     ObjectEditView,
     ObjectImportView,
@@ -57,6 +58,7 @@
     "csv_format",
     "EnhancedPage",
     "EnhancedPaginator",
+    "GenericView",
     "get_csv_form_fields_from_serializer_class",
     "get_paginate_count",
     "GetReturnURLMixin",

nautobot/circuits/api/urls.py

@@ -2,8 +2,7 @@
 
 from . import views
 
-router = OrderedDefaultRouter()
-router.APIRootView = views.CircuitsRootView
+router = OrderedDefaultRouter(view_name="Circuits")
 
 # Providers
 router.register("providers", views.ProviderViewSet)

nautobot/circuits/api/views.py

@@ -1,5 +1,3 @@
-from rest_framework.routers import APIRootView
-
 from nautobot.circuits import filters
 from nautobot.circuits.models import Circuit, CircuitTermination, CircuitType, Provider, ProviderNetwork
 from nautobot.core.models.querysets import count_related
@@ -8,16 +6,6 @@
 
 from . import serializers
 
-
-class CircuitsRootView(APIRootView):
-    """
-    Circuits API root view
-    """
-
-    def get_view_name(self):
-        return "Circuits"
-
-
 #
 # Providers
 #

nautobot/core/api/routers.py

@@ -1,10 +1,23 @@
+import logging
+
 from rest_framework.routers import DefaultRouter
 
+from nautobot.core.api.views import AuthenticatedAPIRootView
+
+logger = logging.getLogger(__name__)
+
 
 class OrderedDefaultRouter(DefaultRouter):
-    def __init__(self, *args, **kwargs):
+    APIRootView = AuthenticatedAPIRootView
+
+    def __init__(self, *args, view_name=None, view_description=None, **kwargs):
         super().__init__(*args, **kwargs)
 
+        self.view_name = view_name
+        if view_name and not view_description:
+            view_description = f"{view_name} API root view"
+        self.view_description = view_description
+
         # Extend the list view mappings to support the DELETE operation
         self.routes[0].mapping.update(
             {
@@ -20,7 +33,18 @@ def get_api_root_view(self, api_urls=None):
         """
         api_root_dict = {}
         list_name = self.routes[0].name
+
         for prefix, _viewset, basename in sorted(self.registry, key=lambda x: x[0]):
             api_root_dict[prefix] = list_name.format(basename=basename)
 
+        if issubclass(self.APIRootView, AuthenticatedAPIRootView):
+            return self.APIRootView.as_view(
+                api_root_dict=api_root_dict, name=self.view_name, description=self.view_description
+            )
+        # Fallback for the established practice of overriding self.APIRootView with a custom class
+        logger.warning(
+            "Something has changed an OrderedDefaultRouter's APIRootView attribute to a custom class. "
+            "Please verify that class %s implements appropriate authentication controls.",
+            self.APIRootView.__name__,
+        )
         return self.APIRootView.as_view(api_root_dict=api_root_dict)

nautobot/core/api/utils.py

@@ -178,6 +178,10 @@ def get_view_name(view, suffix=None):
 
     else:
         # Replicate DRF's built-in behavior.
+        name = getattr(view, "name", None)
+        if name is not None:
+            return view.name
+
         name = view.__class__.__name__
         name = formatting.remove_trailing_string(name, "View")
         name = formatting.remove_trailing_string(name, "ViewSet")

nautobot/core/api/views.py

@@ -22,9 +22,9 @@
 from graphql.execution import ExecutionResult
 from graphql.execution.middleware import MiddlewareManager
 from graphql.type.schema import GraphQLSchema
-from rest_framework import status
+from rest_framework import routers, status
 from rest_framework.exceptions import ParseError, PermissionDenied
-from rest_framework.permissions import AllowAny, IsAuthenticated
+from rest_framework.permissions import IsAuthenticated
 from rest_framework.response import Response
 from rest_framework.reverse import reverse
 from rest_framework.views import APIView
@@ -341,15 +341,25 @@ class ReadOnlyModelViewSet(NautobotAPIVersionMixin, ModelViewSetMixin, ReadOnlyM
 #
 
 
-class APIRootView(NautobotAPIVersionMixin, APIView):
+class AuthenticatedAPIRootView(NautobotAPIVersionMixin, routers.APIRootView):
     """
-    This is the root of the REST API. API endpoints are arranged by app and model name; e.g. `/api/dcim/locations/`.
+    Extends DRF's base APIRootView class to enforce user authentication.
     """
 
-    _ignore_model_permissions = True
+    permission_classes = [IsAuthenticated]
+
+    name = None
+    description = None
+
+
+class APIRootView(AuthenticatedAPIRootView):
+    """
+    This is the root of the REST API.
 
-    def get_view_name(self):
-        return "API Root"
+    API endpoints are arranged by app and model name; e.g. `/api/dcim/locations/`.
+    """
+
+    name = "API Root"
 
     @extend_schema(exclude=True)
     def get(self, request, format=None):  # pylint: disable=redefined-builtin
@@ -492,11 +502,6 @@ class FakeOpenAPIRenderer(OpenApiJsonRenderer):
     @extend_schema(exclude=True)
     def get(self, request, *args, **kwargs):
         """Fix up the rendering of the Swagger UI to work with Nautobot's UI."""
-        if not request.user.is_authenticated:
-            doc_url = reverse("api_docs")
-            login_url = reverse(settings.LOGIN_URL)
-            return redirect(f"{login_url}?next={doc_url}")
-
         # For backward compatibility wtih drf-yasg, `/api/docs/?format=openapi` is a redirect to the JSON schema.
         if request.GET.get("format") == "openapi":
             return redirect("schema_json", permanent=True)
@@ -525,11 +530,12 @@ class NautobotSpectacularRedocView(APIVersioningGetSchemaURLMixin, SpectacularRe
 class GraphQLDRFAPIView(NautobotAPIVersionMixin, APIView):
     """
     API View for GraphQL to integrate properly with DRF authentication mechanism.
-    The code is a stripped down version of graphene-django default View
-    https://github.com/graphql-python/graphene-django/blob/main/graphene_django/views.py#L57
     """
 
-    permission_classes = [AllowAny]
+    # The code is a stripped down version of graphene-django default View
+    # https://github.com/graphql-python/graphene-django/blob/main/graphene_django/views.py#L57
+
+    permission_classes = [IsAuthenticated]
     graphql_schema = None
     executor = None
     backend = None

nautobot/core/settings.py

@@ -242,6 +242,7 @@
     # trim it from all of the individual paths correspondingly.
     # See also https://github.com/nautobot/nautobot-ansible/pull/135 for an example of why this is desirable.
     "SERVERS": [{"url": "/api"}],
+    "SERVE_PERMISSIONS": ["rest_framework.permissions.IsAuthenticated"],
     "SCHEMA_PATH_PREFIX": "/api",
     "SCHEMA_PATH_PREFIX_TRIM": True,
     # use sidecar - locally packaged UI files, not CDN

nautobot/core/tests/integration/test_view_authentication.py

@@ -0,0 +1,67 @@
+from django.test import tag
+
+from nautobot.core.testing import TestCase
+from nautobot.core.utils.lookup import get_url_for_url_pattern, get_url_patterns
+
+
+@tag("integration")
+class AuthenticationEnforcedTestCase(TestCase):
+    r"""
+    Test that all\* registered views require authentication to access.
+
+    \* with a very small number of known exceptions such as login and logout views.
+    """
+
+    def test_all_views_require_authentication(self):
+        self.client.logout()
+        url_patterns = get_url_patterns()
+
+        for url_pattern in url_patterns:
+            with self.subTest(url_pattern=url_pattern):
+                url = get_url_for_url_pattern(url_pattern)
+                response = self.client.get(url, follow=True)
+
+                if response.status_code == 405:  # Method not allowed
+                    response = self.client.post(url, follow=True)
+
+                # Is a view that *should* be open to unauthenticated users?
+                if url in [
+                    "/admin/login/",
+                    "/api/plugins/example-plugin/webhook/",
+                    "/health/",
+                    "/login/",
+                    "/media-failure/",
+                    "/template.css",
+                ]:
+                    self.assertHttpStatus(response, 200, msg=url)
+                elif response.status_code == 200:
+                    # UI views generally should redirect unauthenticated users to the appropriate login page
+                    if url.startswith("/admin"):
+                        if "logout" in url:
+                            # /admin/logout/ sets next=/admin/ because having login redirect to logout would be silly
+                            redirect_url = "/admin/login/?next=/admin/"
+                        else:
+                            redirect_url = f"/admin/login/?next={url}"
+                    else:
+                        if "logout" in url:
+                            # /logout/ sets next=/ because having login redirect back to logout would be silly
+                            redirect_url = "/login/?next=/"
+                        else:
+                            redirect_url = f"/login/?next={url}"
+                    self.assertRedirects(response, redirect_url)
+                elif response.status_code != 403:
+                    if any(
+                        url.startswith(path)
+                        for path in [
+                            "/complete/",  # social auth
+                            "/login/",  # social auth
+                            "/media/",  # MEDIA_ROOT
+                            "/plugins/example-plugin/docs/",  # STATIC_ROOT
+                        ]
+                    ):
+                        self.assertEqual(response.status_code, 404)
+                    else:
+                        self.fail(
+                            f"Unexpected {response.status_code} response at {url}: "
+                            + response.content.decode(response.charset)
+                        )

nautobot/core/tests/test_graphql.py

@@ -630,21 +630,9 @@ def test_graphql_api_token_no_group_exempt(self):
         self.assertEqual(names, ["Rack 1-1", "Rack 1-2", "Rack 2-1", "Rack 2-2"])
 
     def test_graphql_api_no_token(self):
-        """Validate unauthenticated users are not able to query anything by default."""
+        """Validate unauthenticated users are not able to query anything."""
         response = self.client.post(self.api_url, {"query": self.get_racks_query}, format="json")
-        self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertIsInstance(response.data["data"]["racks"], list)
-        names = [item["name"] for item in response.data["data"]["racks"]]
-        self.assertEqual(names, [])
-
-    @override_settings(EXEMPT_VIEW_PERMISSIONS=["*"])
-    def test_graphql_api_no_token_exempt(self):
-        """Validate unauthenticated users are able to query based on the exempt permissions."""
-        response = self.client.post(self.api_url, {"query": self.get_racks_query}, format="json")
-        self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertIsInstance(response.data["data"]["racks"], list)
-        names = [item["name"] for item in response.data["data"]["racks"]]
-        self.assertEqual(names, ["Rack 1-1", "Rack 1-2", "Rack 2-1", "Rack 2-2"])
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
     def test_graphql_api_wrong_token(self):
         """Validate a wrong token return 403."""