Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more detail to location restriction self-test errors. (PP-1557) #2042

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/palace/manager/api/authentication/basic.py
Original file line number Diff line number Diff line change
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 Down Expand Up @@ -412,28 +412,33 @@
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

Check warning on line 418 in src/palace/manager/api/authentication/basic.py

View check run for this annotation

Codecov / codecov/patch

src/palace/manager/api/authentication/basic.py#L417-L418

Added lines #L417 - L418 were not covered by tests

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

Check warning on line 434 in src/palace/manager/api/authentication/basic.py

View check run for this annotation

Codecov / codecov/patch

src/palace/manager/api/authentication/basic.py#L433-L434

Added lines #L433 - L434 were not covered by tests
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
20 changes: 18 additions & 2 deletions src/palace/manager/api/sip/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,18 @@ def remote_authenticate(
self._enforce_patron_location_restriction(info)
return self.info_to_patrondata(info)

def _location_mismatch_message(
self, patron_location: str | None, library_restriction: str | None = None
) -> str:
library_restriction = library_restriction or self.patron_location_restriction
patron_location = (
f"'{patron_location}'" if patron_location is not None else "(missing)"
)
return (
f"Patron location ({patron_location}) does not match "
f"library location restriction ('{library_restriction}')."
)

def _enforce_patron_location_restriction(
self, info: dict[str, Any] | ProblemDetail
) -> None:
Expand All @@ -365,9 +377,13 @@ def _enforce_patron_location_restriction(
if (
not isinstance(info, ProblemDetail)
and self.patron_location_restriction is not None
and info.get("permanent_location") != self.patron_location_restriction
and self.patron_location_restriction != info.get("permanent_location")
):
raise ProblemDetailException(PATRON_OF_ANOTHER_LIBRARY)
raise ProblemDetailException(
PATRON_OF_ANOTHER_LIBRARY.with_debug(
self._location_mismatch_message(info.get("permanent_location"))
)
)

def _run_self_tests(self, _db):
def makeConnection(sip):
Expand Down
15 changes: 12 additions & 3 deletions tests/manager/api/sip/test_authentication_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,9 @@ 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"
patron_location_restriction=library_restriction
)
provider = create_provider(library_settings=library_settings)
client = cast(MockSIPClient, provider.client)
Expand All @@ -364,14 +365,22 @@ def test_remote_authenticate_location_restriction(
client.queue_response(self.end_session_response)
with pytest.raises(ProblemDetailException) as exc:
provider.remote_authenticate("user", "pass")
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY
debug_message = provider._location_mismatch_message(None, library_restriction)
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY.with_debug(
debug_message
)

# This patron has the WRONG location.
client.queue_response(self.evergreen_patron_with_wrong_loc)
client.queue_response(self.end_session_response)
with pytest.raises(ProblemDetailException) as exc:
provider.remote_authenticate("user", "pass")
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY
debug_message = provider._location_mismatch_message(
"OtherLoc", library_restriction
)
assert exc.value.problem_detail == PATRON_OF_ANOTHER_LIBRARY.with_debug(
debug_message
)

def test_encoding(
self,
Expand Down