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.

Sign up for GitHub

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

Handle wrong library ProblemDetailException (PP-1860) #2170

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Handle wrong library ProblemDetailException (PP-1860) #2170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49e1785
Handle auth exception
jonathangreen Nov 15, 2024
1bf5522
Add additional case for test coverage
jonathangreen Nov 15, 2024
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
44 changes: 19 additions & 25 deletions src/palace/manager/api/controller/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,11 @@
from werkzeug.datastructures import Authorization

from palace.manager.api.circulation_exceptions import RemoteInitiatedServerError
from palace.manager.api.problem_details import (
INVALID_CREDENTIALS,
LIBRARY_NOT_FOUND,
REMOTE_INTEGRATION_FAILED,
)
from palace.manager.api.problem_details import INVALID_CREDENTIALS, LIBRARY_NOT_FOUND
from palace.manager.sqlalchemy.model.library import Library
from palace.manager.sqlalchemy.model.patron import Patron
from palace.manager.util.log import LoggerMixin
from palace.manager.util.problem_detail import ProblemDetail
from palace.manager.util.problem_detail import BaseProblemDetailException, ProblemDetail


class BaseCirculationManagerController(LoggerMixin):
Expand Down Expand Up @@ -61,29 +57,23 @@ def authenticated_patron_from_request(self) -> ProblemDetail | Patron | Response
authentication, a ProblemDetail.
"""
# Start off by assuming authentication will not work.
flask.request.patron = None # type: ignore[attr-defined]
setattr(flask.request, "patron", None)

auth = self.authorization_header()

if not auth:
# No credentials were provided.
return self.authenticate()

try:
patron = self.authenticated_patron(auth)
except RemoteInitiatedServerError as e:
return REMOTE_INTEGRATION_FAILED.detailed(
_("Error in authentication service")
)
if patron is None:
# Credentials were provided but they turned out not
# to identify anyone in particular.
return self.authenticate()
patron = self.authenticated_patron(auth)
if isinstance(patron, Patron):
flask.request.patron = patron # type: ignore[attr-defined]
setattr(flask.request, "patron", patron)

return patron

def authenticated_patron(self, authorization_header: Authorization):
def authenticated_patron(
self, authorization_header: Authorization
) -> Patron | ProblemDetail:
"""Look up the patron authenticated by the given authorization header.

The header could contain a barcode and pin or a token for an
Expand All @@ -93,12 +83,16 @@ def authenticated_patron(self, authorization_header: Authorization):

If there's no problem, return a Patron object.
"""
patron = self.manager.auth.authenticated_patron(self._db, authorization_header)
if not patron:
return INVALID_CREDENTIALS

if isinstance(patron, ProblemDetail):
return patron
try:
patron = self.manager.auth.authenticated_patron(
self._db, authorization_header
)
if not patron:
return INVALID_CREDENTIALS
except RemoteInitiatedServerError as e:
return e.problem_detail.detailed(_("Error in authentication service"))
except BaseProblemDetailException as e:
return e.problem_detail

return patron

Expand Down
93 changes: 47 additions & 46 deletions tests/manager/api/controller/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from palace.manager.sqlalchemy.model.resource import Representation
from palace.manager.sqlalchemy.util import create, tuple_to_numericrange
from palace.manager.util.datetime_helpers import utc_now
from palace.manager.util.problem_detail import ProblemDetail
from palace.manager.util.problem_detail import ProblemDetail, ProblemDetailException
from tests.fixtures.api_controller import CirculationControllerFixture
from tests.fixtures.library import LibraryFixture

Expand Down Expand Up @@ -80,56 +80,57 @@ def test_authenticated_patron_from_request(
"/", headers=dict(Authorization=circulation_fixture.valid_auth)
):
result = circulation_fixture.controller.authenticated_patron_from_request()
assert circulation_fixture.default_patron == result
assert circulation_fixture.default_patron == flask.request.patron # type: ignore
assert result == circulation_fixture.default_patron
assert (
getattr(flask.request, "patron") == circulation_fixture.default_patron
)

# No authorization header -> 401 error.
with patch(
"palace.manager.api.controller.base.BaseCirculationManagerController.authorization_header",
lambda x: None,
with (
patch.object(
circulation_fixture.controller,
"authorization_header",
) as mock_auth_header,
circulation_fixture.request_context_with_library("/"),
):
with circulation_fixture.request_context_with_library("/"):
result = (
circulation_fixture.controller.authenticated_patron_from_request()
)
assert isinstance(result, Response)
assert 401 == result.status_code
assert None == flask.request.patron # type: ignore

# Exception contacting the authentication authority -> ProblemDetail
def remote_failure(self, header):
raise RemoteInitiatedServerError("argh", "service")

with patch(
"palace.manager.api.controller.base.BaseCirculationManagerController.authenticated_patron",
remote_failure,
):
with circulation_fixture.request_context_with_library(
mock_auth_header.return_value = None
result = circulation_fixture.controller.authenticated_patron_from_request()
assert isinstance(result, Response)
assert result.status_code == 401
assert getattr(flask.request, "patron") is None

with (
patch.object(
circulation_fixture.manager.auth,
"authenticated_patron",
) as mock_auth_patron,
circulation_fixture.request_context_with_library(
"/", headers=dict(Authorization=circulation_fixture.valid_auth)
):
result = (
circulation_fixture.controller.authenticated_patron_from_request()
)
assert isinstance(result, ProblemDetail)
assert REMOTE_INTEGRATION_FAILED.uri == result.uri
assert "Error in authentication service" == result.detail
assert None == flask.request.patron # type: ignore

# Credentials provided but don't identify anyone in particular
# -> 401 error.
with patch(
"palace.manager.api.controller.base.BaseCirculationManagerController.authenticated_patron",
lambda self, x: None,
),
):
with circulation_fixture.request_context_with_library(
"/", headers=dict(Authorization=circulation_fixture.valid_auth)
):
result = (
circulation_fixture.controller.authenticated_patron_from_request()
)
assert isinstance(result, Response)
assert 401 == result.status_code
assert None == flask.request.patron # type: ignore
# Exception contacting the authentication authority -> ProblemDetail
mock_auth_patron.side_effect = RemoteInitiatedServerError("argh", "service")
result = circulation_fixture.controller.authenticated_patron_from_request()
assert isinstance(result, ProblemDetail)
assert result.uri == REMOTE_INTEGRATION_FAILED.uri
assert result.detail == "Error in authentication service"
assert getattr(flask.request, "patron") is None

# Any other ProblemDetailException -> ProblemDetail
pd = ProblemDetail("uri")
mock_auth_patron.side_effect = ProblemDetailException(pd)
result = circulation_fixture.controller.authenticated_patron_from_request()
assert result is pd
assert getattr(flask.request, "patron") is None

# Credentials provided but don't identify anyone in particular
# -> 401 error.
mock_auth_patron.side_effect = None
mock_auth_patron.return_value = None
result = circulation_fixture.controller.authenticated_patron_from_request()
assert isinstance(result, ProblemDetail)
assert result.status_code == 401
assert getattr(flask.request, "patron") is None

def test_authenticated_patron_invalid_credentials(
self, circulation_fixture: CirculationControllerFixture
Expand Down