Skip to content

Commit

Permalink
Allow restriction to multiple patron locations. (PP-1648) (#2049)
Browse files Browse the repository at this point in the history 
* Add more detail to location restriction self-test errors.

* Allow patron location-based restriction.

* Remove recently added SIP2 single-value patron location restriction.

* Some more tests.
  • Loading branch information
@tdilauro
tdilauro authored Sep 17, 2024
1 parent 98ae71b commit 3069f2e
Show file tree
Hide file tree
Showing 6 changed files with 337 additions and 257 deletions.
1 change: 1 addition & 0 deletions src/palace/manager/api/authentication/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ def __bool__(self):

def __init__(
self,
*,
permanent_id=None,
authorization_identifier=None,
username=None,
Expand Down
145 changes: 98 additions & 47 deletions src/palace/manager/api/authentication/basic.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from collections.abc import Generator
from enum import Enum
from re import Pattern
from typing import Any, TypeVar
from typing import Any, TypeVar, cast

from flask import url_for
from pydantic import PositiveInt, validator
Expand Down Expand Up @@ -41,7 +41,7 @@
from palace.manager.sqlalchemy.model.patron import Patron
from palace.manager.sqlalchemy.util import get_one
from palace.manager.util.log import elapsed_time_logging
from palace.manager.util.problem_detail import ProblemDetail
from palace.manager.util.problem_detail import ProblemDetail, ProblemDetailException


class LibraryIdentifierRestriction(Enum):
Expand All @@ -52,6 +52,11 @@ class LibraryIdentifierRestriction(Enum):
LIST = "list"


class LibraryIdenfitierRestrictionField(Enum):
BARCODE = "barcode"
PATRON_LIBRARY = "patron location"


class Keyboards(Enum):
"""Used by the mobile app to determine which keyboard to display"""

Expand Down Expand Up @@ -223,7 +228,8 @@ class BasicAuthProviderLibrarySettings(AuthProviderLibrarySettings):
"values here. This value is not used if <em>Library Identifier Restriction Type</em> "
"is set to 'No restriction'.",
options={
"barcode": "Barcode",
LibraryIdenfitierRestrictionField.BARCODE: "Barcode",
LibraryIdenfitierRestrictionField.PATRON_LIBRARY: "Patron Location",
},
),
)
Expand Down Expand Up @@ -412,28 +418,33 @@ def testing_patron_or_bust(self, _db: Session) -> tuple[Patron, str | None]:
if self.test_username is None:
raise CannotLoadConfiguration("No test patron identifier is configured.")

patron, password = self.testing_patron(_db)
try:
patron, password = self.testing_patron(_db)
except ProblemDetailException as e:
patron = e.problem_detail

if isinstance(patron, Patron):
return patron, password

debug_message = None
if not patron:
message = (
"Remote declined to authenticate the test patron. "
"The patron may not exist or its password may be wrong."
)
elif isinstance(patron, ProblemDetail):
message = (
"Test patron lookup returned a problem detail - {}: {} ({})".format(
patron.title, patron.detail, patron.uri
)
)
pd = patron
message = f"Test patron lookup returned a problem detail - {pd.title}: {pd.detail} ({pd.uri})"
if pd.debug_message:
message += f" [{pd.debug_message}]"
debug_message = pd.debug_message
else:
message = ( # type: ignore[unreachable]
"Test patron lookup returned invalid value for patron: {!r}".format(
patron
)
)
raise IntegrationException(message)
raise IntegrationException(message, debug_message=debug_message)

def _run_self_tests(self, _db: Session) -> Generator[SelfTestResult, None, None]:
"""Verify the credentials of the test patron for this integration,
Expand Down Expand Up @@ -482,8 +493,6 @@ def authenticate(

# Check that the patron belongs to this library.
patrondata = self.enforce_library_identifier_restriction(patrondata)
if patrondata is None:
return PATRON_OF_ANOTHER_LIBRARY

# At this point we know there is _some_ authenticated patron,
# but it might not correspond to a Patron in our database, and
Expand Down Expand Up @@ -716,39 +725,63 @@ def identifies_individuals(self):
@classmethod
def _restriction_matches(
cls,
field: str | None,
value: str | None,
restriction: str | list[str] | re.Pattern | None,
match_type: LibraryIdentifierRestriction,
) -> bool:
"""Does the given patron match the given restriction?"""
if not field:
# No field -- nothing matches.
return False
) -> tuple[bool, str]:
"""Does the given patron match the given restriction?
if not restriction:
# No restriction -- anything matches.
return True

if match_type == LibraryIdentifierRestriction.REGEX:
if restriction.search(field): # type: ignore[union-attr]
return True
elif match_type == LibraryIdentifierRestriction.PREFIX:
if field.startswith(restriction): # type: ignore[arg-type]
return True
elif match_type == LibraryIdentifierRestriction.STRING:
if field == restriction:
return True
elif match_type == LibraryIdentifierRestriction.LIST:
if field in restriction: # type: ignore[operator]
return True

return False
:param value: The value from the field we're matching against the restriction.
:param restriction: The restriction value.
:param match_type: The type of match we're performing.
:returns: True if the value matches the restriction, False otherwise.
"""
value = value or ""

failure_reason = ""
match [match_type, restriction, value]:
case [LibraryIdentifierRestriction.NONE, *_]:
pass
case [_, _restriction, _] if not _restriction:
pass
case [_, _, _value] if _value is None or _value == "":
failure_reason = "No value in field"
case [LibraryIdentifierRestriction.REGEX, *_]:
if not (_pattern := cast(Pattern, restriction)).search(value):
failure_reason = f"{value!r} does not match regular expression {_pattern.pattern!r}"
case [LibraryIdentifierRestriction.PREFIX, *_]:
if not value.startswith(_string := cast(str, restriction)):
failure_reason = f"{value!r} does not start with {_string!r}"
case [LibraryIdentifierRestriction.STRING, *_]:
if value != (_string := cast(str, restriction)):
failure_reason = f"{value!r} does not exactly match {_string!r}"
case [LibraryIdentifierRestriction.LIST, *_]:
if value not in (_list := cast(list, restriction)):
failure_reason = f"{value!r} not in list {restriction!r}"

return (False, failure_reason) if failure_reason else (True, "")

@staticmethod
def _lookup_in_predefined_field(
patrondata: PatronData, id_field: str
) -> str | None:
predefined_fields = {
k.lower(): v
for k, v in {
LibraryIdenfitierRestrictionField.BARCODE.value: patrondata.authorization_identifier,
LibraryIdenfitierRestrictionField.PATRON_LIBRARY.value: patrondata.library_identifier,
}.items()
}
return predefined_fields.get(id_field.lower())

def get_library_identifier_field_data(
self, patrondata: PatronData
) -> tuple[PatronData, str | None]:
if self.library_identifier_field.lower() == "barcode":
return patrondata, patrondata.authorization_identifier
id_field = self.library_identifier_field
if (
field_value := self._lookup_in_predefined_field(patrondata, id_field)
) is not None:
return patrondata, field_value

if not patrondata.complete:
remote_patrondata = self.remote_patron_lookup(patrondata)
Expand All @@ -757,12 +790,26 @@ def get_library_identifier_field_data(
return patrondata, None
patrondata = remote_patrondata

if (
field_value := self._lookup_in_predefined_field(patrondata, id_field)
) is not None:
return patrondata, field_value

# By default, we'll return the `library_identifier` value.
return patrondata, patrondata.library_identifier

def enforce_library_identifier_restriction(
self, patrondata: PatronData
) -> PatronData | None:
"""Does the given patron match the configured library identifier restriction?"""
) -> PatronData:
"""Does the given patron match the configured library identifier restriction?
If not, raise a ProblemDetail exception.
:param patrondata: A PatronData object.
:returns: A PatronData object.
:raises ProblemDetailException: With `PATRON_OF_ANOTHER_LIBRARY`
ProblemDetail, if the given patron does not match.
"""
if (
self.library_identifier_restriction_type
== LibraryIdentifierRestriction.NONE
Expand All @@ -777,15 +824,19 @@ def enforce_library_identifier_restriction(
# Restriction field is blank, so everything matches.
return patrondata

patrondata, field = self.get_library_identifier_field_data(patrondata)
if self._restriction_matches(
field,
patrondata, field_value = self.get_library_identifier_field_data(patrondata)
is_valid, reason = self._restriction_matches(
field_value,
self.library_identifier_restriction_criteria,
self.library_identifier_restriction_type,
):
return patrondata
else:
return None
)
if not is_valid:
raise ProblemDetailException(
PATRON_OF_ANOTHER_LIBRARY.with_debug(
f"{self.library_identifier_field!r} does not match library restriction: {reason}."
)
)
return patrondata

def authenticated_patron(
self, _db: Session, authorization: dict | str
Expand Down
34 changes: 2 additions & 32 deletions src/palace/manager/api/sip/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,19 +195,6 @@ class SIP2LibrarySettings(BasicAuthProviderLibrarySettings):
description="A specific identifier for the library or branch, if used in patron authentication",
),
)
# Used with SIP2, when it is available in the patron information response.
patron_location_restriction: str | None = FormField(
None,
form=ConfigurationFormItem(
label="Patron Location Restriction",
description=(
"A code for the library or branch, which, when specified, "
"must exactly match the permanent location for the patron."
"<br>If an ILS does not return a location for its patrons, specifying "
"a value here will always result in authentication failure."
),
),
)


class SIP2AuthenticationProvider(
Expand Down Expand Up @@ -257,7 +244,6 @@ def __init__(
self.ssl_verification = settings.ssl_verification
self.dialect = settings.ils
self.institution_id = library_settings.institution_id
self.patron_location_restriction = library_settings.patron_location_restriction
self._client = client

# Check if patrons should be blocked based on SIP status
Expand Down Expand Up @@ -349,26 +335,8 @@ def remote_authenticate(
# passing it on.
password = None
info = self.patron_information(username, password)
self._enforce_patron_location_restriction(info)
return self.info_to_patrondata(info)

def _enforce_patron_location_restriction(
self, info: dict[str, Any] | ProblemDetail
) -> None:
"""Raise an exception if patron location does not match the restriction.
If a location restriction is specified for the library against which the
patron is attempting to authenticate, then the authentication will fail
if either (1) the patron does not have an associated location or (2) the
patron's location does not exactly match the one configured.
"""
if (
not isinstance(info, ProblemDetail)
and self.patron_location_restriction is not None
and info.get("permanent_location") != self.patron_location_restriction
):
raise ProblemDetailException(PATRON_OF_ANOTHER_LIBRARY)

def _run_self_tests(self, _db):
def makeConnection(sip):
sip.connect()
Expand Down Expand Up @@ -449,6 +417,8 @@ def info_to_patrondata(
patrondata.email_address = info["email_address"]
if "personal_name" in info:
patrondata.personal_name = info["personal_name"]
if "permanent_location" in info:
patrondata.library_identifier = info["permanent_location"]
if "fee_amount" in info:
fines = info["fee_amount"]
else:
Expand Down
25 changes: 20 additions & 5 deletions tests/manager/api/sip/test_authentication_provider.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
from palace.manager.api.authentication.basic import (
BasicAuthProviderLibrarySettings,
Keyboards,
LibraryIdenfitierRestrictionField,
LibraryIdentifierRestriction,
)
from palace.manager.api.problem_details import (
INVALID_CREDENTIALS,
Expand Down Expand Up @@ -346,8 +348,11 @@ def test_remote_authenticate_location_restriction(
create_library_settings: Callable[..., SIP2Settings],
):
# This patron authentication library instance is configured with "TestLoc".
library_restriction = "TestLoc"
library_settings = create_library_settings(
patron_location_restriction="TestLoc"
library_identifier_restriction_type=LibraryIdentifierRestriction.STRING,
library_identifier_field=LibraryIdenfitierRestrictionField.PATRON_LIBRARY.value,
library_identifier_restriction_criteria=library_restriction,
)
provider = create_provider(library_settings=library_settings)
client = cast(MockSIPClient, provider.client)
Expand All @@ -357,21 +362,31 @@ def test_remote_authenticate_location_restriction(
client.queue_response(self.end_session_response)
patrondata = provider.remote_authenticate("user", "pass")
assert isinstance(patrondata, PatronData)
patrondata = provider.enforce_library_identifier_restriction(patrondata)
assert isinstance(patrondata, PatronData)
assert "Patron Name" == patrondata.personal_name

# This patron does NOT have an associated location.
client.queue_response(self.evergreen_patron_wo_location)
client.queue_response(self.end_session_response)
patrondata = provider.remote_authenticate("user", "pass")
assert isinstance(patrondata, PatronData)
with pytest.raises(ProblemDetailException) as exc:
provider.remote_authenticate("user", "pass")
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY
provider.enforce_library_identifier_restriction(patrondata)
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY.with_debug(
"'patron location' does not match library restriction: No value in field."
)

# This patron has the WRONG location.
client.queue_response(self.evergreen_patron_with_wrong_loc)
client.queue_response(self.end_session_response)
patrondata = provider.remote_authenticate("user", "pass")
assert isinstance(patrondata, PatronData)
with pytest.raises(ProblemDetailException) as exc:
provider.remote_authenticate("user", "pass")
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY
provider.enforce_library_identifier_restriction(patrondata)
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY.with_debug(
"'patron location' does not match library restriction: 'OtherLoc' does not exactly match 'TestLoc'."
)

def test_encoding(
self,
Expand Down
Loading

0 comments on commit 3069f2e

Please sign in to comment.