Clean up some PatronController-related types. (#2162)

pyproject.toml

@@ -75,8 +75,10 @@ module = [
     "palace.manager.api.admin.controller.integration_settings",
     "palace.manager.api.admin.controller.library_settings",
     "palace.manager.api.admin.controller.metadata_services",
+    "palace.manager.api.admin.controller.patron",
     "palace.manager.api.admin.controller.patron_auth_services",
     "palace.manager.api.admin.controller.report",
+    "palace.manager.api.admin.controller.util",
     "palace.manager.api.admin.dashboard_stats",
     "palace.manager.api.admin.form_data",
     "palace.manager.api.admin.model.dashboard_statistics",

src/palace/manager/api/admin/controller/base.py

@@ -18,9 +18,11 @@
 from palace.manager.sqlalchemy.model.admin import Admin, AdminRole
 from palace.manager.sqlalchemy.model.library import Library
 from palace.manager.sqlalchemy.util import get_one, get_one_or_create
+from palace.manager.util.log import LoggerMixin
+from palace.manager.util.problem_detail import ProblemDetail
 
 
-class AdminController:
+class AdminController(LoggerMixin):
     def __init__(self, manager):
         self.manager = manager
         self._db = self.manager._db
@@ -60,7 +62,7 @@ def authenticated_admin_from_request(self):
         flask.request.admin = None
         return INVALID_ADMIN_CREDENTIALS
 
-    def authenticated_admin(self, admin_details):
+    def authenticated_admin(self, admin_details) -> Admin:
         """Creates or updates an admin with the given details"""
 
         admin, is_new = get_one_or_create(self._db, Admin, email=admin_details["email"])
@@ -92,7 +94,7 @@ def authenticated_admin(self, admin_details):
 
         return admin
 
-    def check_csrf_token(self):
+    def check_csrf_token(self) -> str | ProblemDetail:
         """Verifies that the CSRF token in the form data or X-CSRF-Token header
         matches the one in the session cookie.
         """
@@ -102,34 +104,36 @@ def check_csrf_token(self):
             return INVALID_CSRF_TOKEN
         return cookie_token
 
-    def get_csrf_token(self):
+    @staticmethod
+    def get_csrf_token() -> str | None:
         """Returns the CSRF token for the current session."""
         return flask.request.cookies.get("csrf_token")
 
-    def generate_csrf_token(self):
+    @staticmethod
+    def generate_csrf_token() -> str:
         """Generate a random CSRF token."""
         return base64.b64encode(os.urandom(24)).decode("utf-8")
 
 
 class AdminPermissionsControllerMixin:
     """Mixin that provides methods for verifying an admin's roles."""
 
-    def require_system_admin(self):
-        admin = getattr(flask.request, "admin", None)
+    def require_system_admin(self) -> None:
+        admin: Admin | None = getattr(flask.request, "admin", None)
         if not admin or not admin.is_system_admin():
             raise AdminNotAuthorized()
 
-    def require_sitewide_library_manager(self):
-        admin = getattr(flask.request, "admin", None)
+    def require_sitewide_library_manager(self) -> None:
+        admin: Admin | None = getattr(flask.request, "admin", None)
         if not admin or not admin.is_sitewide_library_manager():
             raise AdminNotAuthorized()
 
-    def require_library_manager(self, library):
-        admin = getattr(flask.request, "admin", None)
+    def require_library_manager(self, library: Library) -> None:
+        admin: Admin | None = getattr(flask.request, "admin", None)
         if not admin or not admin.is_library_manager(library):
             raise AdminNotAuthorized()
 
-    def require_librarian(self, library):
-        admin = getattr(flask.request, "admin", None)
+    def require_librarian(self, library: Library) -> None:
+        admin: Admin | None = getattr(flask.request, "admin", None)
         if not admin or not admin.is_librarian(library):
             raise AdminNotAuthorized()

src/palace/manager/api/admin/controller/patron.py

@@ -1,10 +1,13 @@
 from __future__ import annotations
 
+from typing import Any
+
 import flask
 from flask import Response
 from flask_babel import lazy_gettext as _
 
 from palace.manager.api.admin.controller.base import AdminPermissionsControllerMixin
+from palace.manager.api.admin.controller.util import required_library_from_request
 from palace.manager.api.admin.problem_details import NO_SUCH_PATRON
 from palace.manager.api.adobe_vendor_id import AuthdataUtility
 from palace.manager.api.authentication.base import CannotCreateLocalPatron, PatronData
@@ -16,27 +19,27 @@
 
 
 class PatronController(CirculationManagerController, AdminPermissionsControllerMixin):
-    def _load_patrondata(self, authenticator=None):
+    def _load_patron_data(
+        self, authenticator: LibraryAuthenticator | None = None
+    ) -> PatronData | ProblemDetail:
         """Extract a patron identifier from an incoming form submission,
         and ask the library's LibraryAuthenticator to turn it into a
         PatronData by doing a remote lookup in the ILS.
 
         :param authenticator: A LibraryAuthenticator. This is for mocking
         during tests; it's not necessary to provide it normally.
         """
-        self.require_librarian(flask.request.library)
+        library = required_library_from_request(flask.request)
+        self.require_librarian(library)
 
         identifier = flask.request.form.get("identifier")
         if not identifier:
             return NO_SUCH_PATRON.detailed(_("Please enter a patron identifier"))
 
         if not authenticator:
-            authenticator = LibraryAuthenticator.from_config(
-                self._db, flask.request.library
-            )
+            authenticator = LibraryAuthenticator.from_config(self._db, library)
 
         patron_data = PatronData(authorization_identifier=identifier)
-        remote_patron_data = None
         patron_lookup_providers = list(authenticator.unique_patron_lookup_providers)
 
         if not patron_lookup_providers:
@@ -45,50 +48,53 @@ def _load_patrondata(self, authenticator=None):
             )
 
         for provider in patron_lookup_providers:
-            remote_patron_data = provider.remote_patron_lookup(patron_data)
-            if remote_patron_data:
-                return remote_patron_data
+            if lookup := getattr(provider, "remote_patron_lookup", None):
+                remote_patron_data: PatronData | None = lookup(patron_data)
+                if remote_patron_data:
+                    return remote_patron_data
 
         # If we get here, none of the providers succeeded.
-        if not remote_patron_data:
-            return NO_SUCH_PATRON.detailed(
-                _(
-                    "No patron with identifier %(patron_identifier)s was found at your library",
-                    patron_identifier=identifier,
-                ),
-            )
+        return NO_SUCH_PATRON.detailed(
+            _(
+                "No patron with identifier %(patron_identifier)s was found at your library",
+                patron_identifier=identifier,
+            ),
+        )
 
-    def lookup_patron(self, authenticator=None):
+    def lookup_patron(
+        self, authenticator: LibraryAuthenticator | None = None
+    ) -> dict[str, Any] | ProblemDetail:
         """Look up personal information about a patron via the ILS.
 
         :param authenticator: A LibraryAuthenticator. This is for mocking
             during tests; it's not necessary to provide it normally.
         """
-        patrondata = self._load_patrondata(authenticator)
-        if isinstance(patrondata, ProblemDetail):
-            return patrondata
-        return patrondata.to_dict
-
-    def reset_adobe_id(self, authenticator=None):
+        patron_data: PatronData | ProblemDetail = self._load_patron_data(authenticator)
+        if isinstance(patron_data, ProblemDetail):
+            return patron_data
+        return patron_data.to_dict
+
+    def reset_adobe_id(
+        self, authenticator: LibraryAuthenticator | None = None
+    ) -> Response | ProblemDetail:
         """Delete all Credentials for a patron that are relevant
         to the patron's Adobe Account ID.
 
         :param authenticator: A LibraryAuthenticator. This is for mocking
             during tests; it's not necessary to provide it normal
         """
-        patrondata = self._load_patrondata(authenticator)
-        if isinstance(patrondata, ProblemDetail):
-            return patrondata
+        library = required_library_from_request(flask.request)
+        patron_data = self._load_patron_data(authenticator)
+        if isinstance(patron_data, ProblemDetail):
+            return patron_data
         # Turn the Identifier into a Patron object.
         try:
-            patron, is_new = patrondata.get_or_create_patron(
-                self._db, flask.request.library.id
-            )
-        except CannotCreateLocalPatron as e:
+            patron, is_new = patron_data.get_or_create_patron(self._db, library.id)
+        except CannotCreateLocalPatron:
             return NO_SUCH_PATRON.detailed(
                 _(
                     "Could not create local patron object for %(patron_identifier)s",
-                    patron_identifier=patrondata.authorization_identifier,
+                    patron_identifier=patron_data.authorization_identifier,
                 )
             )
 
@@ -98,7 +104,7 @@ def reset_adobe_id(self, authenticator=None):
         if patron.username:
             identifier = patron.username
         else:
-            identifier = "with identifier " + patron.authorization_identifier
+            identifier = f"with identifier {patron.authorization_identifier}"
         return Response(
             str(
                 _(

src/palace/manager/api/authentication/base.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 from abc import ABC, abstractmethod
-from typing import TypeVar
+from typing import Any, TypeVar
 
 from sqlalchemy.orm import Session
 from werkzeug.datastructures import Authorization
@@ -475,7 +475,7 @@ def to_response_parameters(self):
         return {}
 
     @property
-    def to_dict(self):
+    def to_dict(self) -> dict[str, Any]:
         """Convert the information in this PatronData to a dictionary
         which can be converted to JSON and sent out to a client.
         """

tests/manager/api/admin/controller/test_patron.py

@@ -1,3 +1,5 @@
+from unittest.mock import MagicMock
+
 import flask
 import pytest
 from werkzeug.datastructures import ImmutableMultiDict
@@ -7,7 +9,9 @@
 from palace.manager.api.admin.problem_details import NO_SUCH_PATRON
 from palace.manager.api.adobe_vendor_id import AuthdataUtility
 from palace.manager.api.authentication.base import PatronData
+from palace.manager.api.authenticator import LibraryAuthenticator
 from palace.manager.sqlalchemy.model.admin import AdminRole
+from palace.manager.util.problem_detail import ProblemDetail
 from tests.fixtures.api_admin import AdminControllerFixture
 from tests.fixtures.api_controller import ControllerFixture
 
@@ -29,23 +33,23 @@ class TestPatronController:
     def test__load_patrondata(self, patron_controller_fixture: PatronControllerFixture):
         """Test the _load_patrondata helper method."""
 
-        class MockAuthenticator:
-            def __init__(self, providers):
-                self.unique_patron_lookup_providers = providers
+        def mock_authenticator(providers):
+            mock_authenticator_ = MagicMock(spec=LibraryAuthenticator)
+            mock_authenticator_.unique_patron_lookup_providers = providers
+            return mock_authenticator_
 
         class MockAuthenticationProvider:
-            def __init__(self, patron_dict):
-                self.patron_dict = patron_dict
+            def __init__(self, patron_data_dict: dict[str, PatronData]):
+                self.patron_dict = patron_data_dict
 
-            def remote_patron_lookup(self, patrondata):
+            def remote_patron_lookup(self, patrondata) -> PatronData | None:
                 return self.patron_dict.get(patrondata.authorization_identifier)
 
-        authenticator = MockAuthenticator([])
-        auth_provider = MockAuthenticationProvider({})
+        authenticator = mock_authenticator([])
         identifier = "Patron"
 
         form = ImmutableMultiDict([("identifier", identifier)])
-        m = patron_controller_fixture.manager.admin_patron_controller._load_patrondata
+        m = patron_controller_fixture.manager.admin_patron_controller._load_patron_data
 
         # User doesn't have admin permission
         with patron_controller_fixture.ctrl.request_context_with_library("/"):
@@ -71,7 +75,8 @@ def remote_patron_lookup(self, patrondata):
             )
 
         # Authenticator can't find patron with this identifier
-        authenticator.unique_patron_lookup_providers.append(auth_provider)
+        auth_provider = MockAuthenticationProvider({})
+        authenticator = mock_authenticator([auth_provider])
         with patron_controller_fixture.request_context_with_library_and_admin("/"):
             flask.request.form = form
             response = m(authenticator)
@@ -85,15 +90,15 @@ def remote_patron_lookup(self, patrondata):
 
         # Authenticator can find patron with this identifier
         auth_provider = MockAuthenticationProvider(
-            {identifier: {"authorization_identifier": identifier}}
+            {identifier: PatronData(authorization_identifier=identifier)}
         )
-        authenticator.unique_patron_lookup_providers.clear()
-        authenticator.unique_patron_lookup_providers.append(auth_provider)
+        authenticator = mock_authenticator([auth_provider])
 
         with patron_controller_fixture.request_context_with_library_and_admin("/"):
             flask.request.form = form
             response = m(authenticator)
-            assert identifier == response["authorization_identifier"]
+            assert not isinstance(response, ProblemDetail)
+            assert identifier == response.authorization_identifier
 
     def test_lookup_patron(self, patron_controller_fixture: PatronControllerFixture):
         # Here's a patron.
@@ -103,7 +108,7 @@ def test_lookup_patron(self, patron_controller_fixture: PatronControllerFixture)
         # This PatronController will always return information about that
         # patron, no matter what it's asked for.
         class MockPatronController(PatronController):
-            def _load_patrondata(self, authenticator):
+            def _load_patron_data(self, authenticator):
                 self.called_with = authenticator
                 return PatronData(
                     authorization_identifier="An Identifier",
@@ -112,7 +117,7 @@ def _load_patrondata(self, authenticator):
 
         controller = MockPatronController(patron_controller_fixture.manager)
 
-        authenticator = object()
+        authenticator = MagicMock()
         with patron_controller_fixture.request_context_with_library_and_admin("/"):
             response = controller.lookup_patron(authenticator)
             # The authenticator was passed into _load_patrondata()
@@ -121,6 +126,7 @@ def _load_patrondata(self, authenticator):
             # _load_patrondata() returned a PatronData object. We
             # converted it to a dictionary, which will be dumped to
             # JSON on the way out.
+            assert not isinstance(response, ProblemDetail)
             assert "An Identifier" == response["authorization_identifier"]
             assert "A Patron" == response["personal_name"]
 
@@ -138,7 +144,7 @@ def test_reset_adobe_id(self, patron_controller_fixture: PatronControllerFixture
         class MockPatronController(PatronController):
             mock_patrondata: PatronData | None = None
 
-            def _load_patrondata(self, authenticator):
+            def _load_patron_data(self, authenticator):
                 self.called_with = authenticator
                 return self.mock_patrondata
 
@@ -148,7 +154,7 @@ def _load_patrondata(self, authenticator):
         )
 
         # We reset their Adobe ID.
-        authenticator = object()
+        authenticator = MagicMock()
         with patron_controller_fixture.request_context_with_library_and_admin("/"):
             form = ImmutableMultiDict([("identifier", patron.authorization_identifier)])
             flask.request.form = form
@@ -170,6 +176,8 @@ def _load_patrondata(self, authenticator):
             flask.request.form = form
             response = controller.reset_adobe_id(authenticator)
 
+            assert isinstance(response, ProblemDetail)
             assert 404 == response.status_code
             assert NO_SUCH_PATRON.uri == response.uri
+            assert response.detail is not None
             assert "Could not create local patron object" in response.detail