diff --git a/pyproject.toml b/pyproject.toml index 35faa2fca..4d1b07540 100644 --- a/pyproject.toml +++ b/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", diff --git a/src/palace/manager/api/admin/controller/base.py b/src/palace/manager/api/admin/controller/base.py index 57256c6b5..7f7727705 100644 --- a/src/palace/manager/api/admin/controller/base.py +++ b/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,11 +104,13 @@ 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") @@ -114,22 +118,22 @@ def generate_csrf_token(self): 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() diff --git a/src/palace/manager/api/admin/controller/patron.py b/src/palace/manager/api/admin/controller/patron.py index 28e376250..85c10b605 100644 --- a/src/palace/manager/api/admin/controller/patron.py +++ b/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,7 +19,9 @@ 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. @@ -24,19 +29,17 @@ def _load_patrondata(self, authenticator=None): :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( _( diff --git a/src/palace/manager/api/authentication/base.py b/src/palace/manager/api/authentication/base.py index b2390bf2c..473cbb974 100644 --- a/src/palace/manager/api/authentication/base.py +++ b/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. """ diff --git a/tests/manager/api/admin/controller/test_patron.py b/tests/manager/api/admin/controller/test_patron.py index 21eb9607b..65a2354b5 100644 --- a/tests/manager/api/admin/controller/test_patron.py +++ b/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