From e6d3806cc4a9d936738882eebebc7c1c936b732b Mon Sep 17 00:00:00 2001 From: ttuovinen Date: Wed, 7 Feb 2024 14:29:35 +0200 Subject: [PATCH] Merge changes from origin/main to upstream_v14.1.0 --- ...d7ef6948af4e_create_loancheckouts_table.py | 53 +++++++++++++++ api/authenticator.py | 9 +-- api/circulation.py | 14 ++++ api/ekirjasto_authentication.py | 66 ++++++++++--------- api/ekirjasto_controller.py | 23 +++---- core/model/__init__.py | 1 + core/model/patron.py | 26 ++++++++ poetry.lock | 13 +++- pyproject.toml | 1 + tests/api/finland/test_ekirjasto.py | 20 +++--- .../test_opensearch_analytics_search.py | 2 +- tests/api/test_circulationapi.py | 45 +++++++++++++ 12 files changed, 210 insertions(+), 63 deletions(-) create mode 100644 alembic/versions/20240205_d7ef6948af4e_create_loancheckouts_table.py diff --git a/alembic/versions/20240205_d7ef6948af4e_create_loancheckouts_table.py b/alembic/versions/20240205_d7ef6948af4e_create_loancheckouts_table.py new file mode 100644 index 000000000..49279774d --- /dev/null +++ b/alembic/versions/20240205_d7ef6948af4e_create_loancheckouts_table.py @@ -0,0 +1,53 @@ +"""Create loancheckouts table + +Revision ID: d7ef6948af4e +Revises: cc084e35e037 +Create Date: 2024-02-05 08:45:20.164531+00:00 + +""" +import sqlalchemy as sa + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "d7ef6948af4e" +down_revision = "cc084e35e037" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "loancheckouts", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("patron_id", sa.Integer(), nullable=True), + sa.Column("license_pool_id", sa.Integer(), nullable=True), + sa.Column("timestamp", sa.DateTime(timezone=True), nullable=True), + sa.ForeignKeyConstraint( + ["license_pool_id"], + ["licensepools.id"], + ), + sa.ForeignKeyConstraint(["patron_id"], ["patrons.id"], ondelete="CASCADE"), + sa.PrimaryKeyConstraint("id"), + ) + op.create_index( + op.f("ix_loancheckouts_license_pool_id"), + "loancheckouts", + ["license_pool_id"], + unique=False, + ) + op.create_index( + op.f("ix_loancheckouts_patron_id"), "loancheckouts", ["patron_id"], unique=False + ) + op.create_index( + op.f("ix_loancheckouts_timestamp"), "loancheckouts", ["timestamp"], unique=False + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + op.drop_index(op.f("ix_loancheckouts_timestamp"), table_name="loancheckouts") + op.drop_index(op.f("ix_loancheckouts_patron_id"), table_name="loancheckouts") + op.drop_index(op.f("ix_loancheckouts_license_pool_id"), table_name="loancheckouts") + op.drop_table("loancheckouts") + # ### end Alembic commands ### diff --git a/api/authenticator.py b/api/authenticator.py index c11bdedd8..1b2faf7ee 100644 --- a/api/authenticator.py +++ b/api/authenticator.py @@ -431,7 +431,7 @@ def register_ekirjasto_provider( self.ekirjasto_provider = provider # Finland - def get_ekirjasto_provider(self) -> EkirjastoAuthenticationAPI: + def get_ekirjasto_provider(self) -> EkirjastoAuthenticationAPI | None: return self.ekirjasto_provider @property @@ -495,9 +495,10 @@ def authenticated_patron( return INVALID_EKIRJASTO_DELEGATE_TOKEN provider = self.ekirjasto_provider # Get decoded payload from the delegate token. - provider_token = provider.validate_ekirjasto_delegate_token(auth.token) - if isinstance(provider_token, ProblemDetail): - return provider_token + validate_result = provider.validate_ekirjasto_delegate_token(auth.token) + if isinstance(validate_result, ProblemDetail): + return validate_result + provider_token = validate_result elif auth.type.lower() == "bearer": # The patron wants to use an # SAMLAuthenticationProvider. Figure out which one. diff --git a/api/circulation.py b/api/circulation.py index e063264be..51a0168ff 100644 --- a/api/circulation.py +++ b/api/circulation.py @@ -46,6 +46,7 @@ get_one, ) from core.model.integration import IntegrationConfiguration +from core.model.patron import LoanCheckout from core.util.datetime_helpers import utc_now from core.util.log import LoggerMixin @@ -891,6 +892,18 @@ def _collect_event( library, licensepool, name, neighborhood=neighborhood ) + # Finland + def _collect_checkout_history( + self, patron: Patron, license_pool: LicensePool + ) -> None: + """Save history for checkout, for later use in loan history or analytics""" + __transaction = self._db.begin_nested() + checkout = LoanCheckout( + patron=patron, license_pool=license_pool, timestamp=utc_now() + ) + self._db.add(checkout) + __transaction.commit() + def _collect_checkout_event(self, patron: Patron, licensepool: LicensePool) -> None: """A simple wrapper around _collect_event for handling checkouts. @@ -1104,6 +1117,7 @@ def borrow( # Send out an analytics event to record the fact that # a loan was initiated through the circulation # manager. + self._collect_checkout_history(patron, licensepool) self._collect_checkout_event(patron, licensepool) return loan, None, new_loan_record diff --git a/api/ekirjasto_authentication.py b/api/ekirjasto_authentication.py index dc5f6c7d7..aa549712a 100644 --- a/api/ekirjasto_authentication.py +++ b/api/ekirjasto_authentication.py @@ -110,8 +110,8 @@ def __init__( self.ekirjasto_environment = settings.ekirjasto_environment self.delegate_expire_timemestamp = settings.delegate_expire_time - self.delegate_token_signing_secret = None - self.delegate_token_encrypting_secret = None + self.delegate_token_signing_secret: str | None = None + self.delegate_token_encrypting_secret: bytes | None = None self.analytics = analytics @@ -284,7 +284,7 @@ def get_credential_from_header(self, auth: Authorization) -> str | None: # circulation API needs additional authentication. return None - def get_patron_delegate_id(self, _db: Session, patron: Patron) -> str: + def get_patron_delegate_id(self, _db: Session, patron: Patron) -> str | None: """Find or randomly create an identifier to use when identifying this patron from delegate token. """ @@ -343,17 +343,6 @@ def set_secrets(self, _db): _db.commit() self.delegate_token_encrypting_secret = secret.value.encode() - def _check_secrets_or_throw(self): - if ( - self.delegate_token_signing_secret == None - or len(self.delegate_token_signing_secret) == 0 - or self.delegate_token_encrypting_secret == None - or len(self.delegate_token_encrypting_secret) == 0 - ): - raise InternalServerError( - "Ekirjasto authenticator not fully setup, secrets are missing." - ) - def create_ekirjasto_delegate_token( self, provider_token: str, patron_delegate_id: str, expires: int ) -> str: @@ -362,7 +351,15 @@ def create_ekirjasto_delegate_token( The patron will use this as the authentication toekn to authentiacte againsy circulation backend. """ - self._check_secrets_or_throw() + if not self.delegate_token_encrypting_secret: + raise InternalServerError( + "Error creating delegate token, encryption secret missing" + ) + + if not self.delegate_token_signing_secret: + raise InternalServerError( + "Error creating delegate token, signing secret missing" + ) # Encrypt the ekirjasto token with a128cbc-hs256 algorithm. fernet = Fernet(self.delegate_token_encrypting_secret) @@ -377,6 +374,7 @@ def create_ekirjasto_delegate_token( iat=int(utc_now().timestamp()), exp=expires, ) + return jwt.encode( payload, self.delegate_token_signing_secret, algorithm="HS256" ) @@ -392,7 +390,10 @@ def decode_ekirjasto_delegate_token( return decoded payload """ - self._check_secrets_or_throw() + if not self.delegate_token_signing_secret: + raise InternalServerError( + "Error decoding delegate token, signing secret missing" + ) options = dict( verify_signature=True, @@ -418,6 +419,10 @@ def decode_ekirjasto_delegate_token( return decoded_payload def _decrypt_ekirjasto_token(self, token: str): + if not self.delegate_token_encrypting_secret: + raise InternalServerError( + "Error decrypting ekirjasto token, signing secret missing" + ) fernet = Fernet(self.delegate_token_encrypting_secret) encrypted_token = b64decode(token.encode("ascii")) return fernet.decrypt(encrypted_token).decode() @@ -445,7 +450,9 @@ def validate_ekirjasto_delegate_token( return INVALID_EKIRJASTO_DELEGATE_TOKEN return decoded_payload - def remote_refresh_token(self, token: str) -> (str, int): + def remote_refresh_token( + self, token: str + ) -> tuple[ProblemDetail, None] | tuple[str, int]: """Refresh ekirjasto token with ekirjasto API call. We assume that the token is valid, API call fails if not. @@ -454,9 +461,9 @@ def remote_refresh_token(self, token: str) -> (str, int): """ if self.ekirjasto_environment == EkirjastoEnvironment.FAKE: - token = self.fake_ekirjasto_token - expires = utc_now() + datetime.timedelta(days=1) - return token, expires.timestamp() + fake_token = self.fake_ekirjasto_token + expire_date = utc_now() + datetime.timedelta(days=1) + return fake_token, int(expire_date.timestamp()) url = self._ekirjasto_api_url + "/v1/auth/refresh" @@ -469,10 +476,6 @@ def remote_refresh_token(self, token: str) -> (str, int): # Do nothing if authentication fails, e.g. token expired. return INVALID_EKIRJASTO_TOKEN, None elif response.status_code != 200: - msg = "Got unexpected response code %d. Content: %s" % ( - response.status_code, - response.content, - ) return EKIRJASTO_REMOTE_AUTHENTICATION_FAILED, None else: try: @@ -482,6 +485,7 @@ def remote_refresh_token(self, token: str) -> (str, int): token = content["token"] expires = content["exp"] + return token, expires def remote_patron_lookup( @@ -532,8 +536,6 @@ def remote_patron_lookup( return self._userinfo_to_patrondata(content) - return EKIRJASTO_REMOTE_AUTHENTICATION_FAILED - def remote_authenticate( self, ekirjasto_token: str | None ) -> PatronData | ProblemDetail | None: @@ -608,7 +610,7 @@ def local_patron_lookup( def ekirjasto_authenticate( self, _db: Session, ekirjasto_token: str - ) -> (Patron, bool): + ) -> tuple[PatronData | Patron | ProblemDetail | None, bool]: """Authenticate patron with remote ekirjasto API and if necessary, create authenticated patron if not in database. @@ -620,17 +622,17 @@ def ekirjasto_authenticate( log_method=self.logger().info, message_prefix="authenticated_patron - ekirjasto_authenticate", ): - patron = self.authenticate_and_update_patron(_db, ekirjasto_token) + auth_result = self.authenticate_and_update_patron(_db, ekirjasto_token) - if isinstance(patron, PatronData): + if isinstance(auth_result, PatronData): # We didn't find the patron, but authentication to external truth was - # succesfull, so we create a new patron with the information we have. - patron, is_new = patron.get_or_create_patron( + # successful, so we create a new patron with the information we have. + patron, is_new = auth_result.get_or_create_patron( _db, self.library_id, analytics=self.analytics ) patron.last_external_sync = utc_now() - return patron, is_new + return auth_result, is_new def authenticated_patron( self, _db: Session, authorization: dict | str diff --git a/api/ekirjasto_controller.py b/api/ekirjasto_controller.py index 986f02df1..72974b0eb 100644 --- a/api/ekirjasto_controller.py +++ b/api/ekirjasto_controller.py @@ -34,35 +34,30 @@ def __init__(self, circulation_manager, authenticator): self._logger = logging.getLogger(__name__) - def _get_delegate_expire_timestamp(self, ekirjasto_token_expires: int) -> int: + def _get_delegate_expire_timestamp(self, ekirjasto_expire_millis: int) -> int: """Get the expire time to use for delegate token, it is calculated based on expire time of the ekirjasto token. - :param ekirjasto_token_expires: Ekirjasto token expiration timestamp in milliseconds. + :param ekirjasto_expire_millis: Ekirjasto token expiration timestamp in milliseconds. - :return: Timestamp for the delegate token expiration. + :return: Timestamp for the delegate token expiration in seconds. """ - # Ekirjasto expire is in milliseconds but JWT uses seconds. - ekirjasto_token_expires = int(ekirjasto_token_expires / 1000) - - delegate_token_expires = ( + delegate_expire_seconds = ( utc_now() + datetime.timedelta( seconds=self._authenticator.ekirjasto_provider.delegate_expire_timemestamp ) ).timestamp() - # Use ekirjasto expire time at 70 % of the remaining duration, so we have some time to refresh it. now_seconds = utc_now().timestamp() - ekirjasto_token_expires = ( - ekirjasto_token_expires - now_seconds - ) * 0.7 + now_seconds - if ekirjasto_token_expires < delegate_token_expires: - return int(ekirjasto_token_expires) + # Use ekirjasto expire time at 70 % of the remaining duration, so we have some time to refresh it. + ekirjasto_expire_seconds = ( + (ekirjasto_expire_millis / 1000) - now_seconds + ) * 0.7 + now_seconds - return int(delegate_token_expires) + return int(min(ekirjasto_expire_seconds, delegate_expire_seconds)) def get_tokens(self, authorization, validate_expire=False): """Extract possible delegate and ekirjasto tokens from the authorization header.""" diff --git a/core/model/__init__.py b/core/model/__init__.py index 6778bde80..57333ca83 100644 --- a/core/model/__init__.py +++ b/core/model/__init__.py @@ -560,6 +560,7 @@ def _bulk_operation(self): Hold, Loan, LoanAndHoldMixin, + LoanCheckout, Patron, PatronProfileStorage, ) diff --git a/core/model/patron.py b/core/model/patron.py index 445657991..4a0f3220f 100644 --- a/core/model/patron.py +++ b/core/model/patron.py @@ -156,6 +156,11 @@ class Patron(Base): loans: Mapped[list[Loan]] = relationship( "Loan", backref="patron", cascade="delete", uselist=True ) + + loan_checkouts: Mapped[list[LoanCheckout]] = relationship( + "LoanCheckout", back_populates="patron", cascade="delete", uselist=True + ) + holds: Mapped[list[Hold]] = relationship( "Hold", back_populates="patron", @@ -571,6 +576,27 @@ def until(self, default_loan_period): return start + default_loan_period +# Finland +class LoanCheckout(Base, LoanAndHoldMixin): + """A model to keep track of loan history, i.e. past checkouts. Similar to `Loan` model with some fields omitted and timestamp added""" + + __tablename__ = "loancheckouts" + id = Column(Integer, primary_key=True) + + patron_id = Column( + Integer, ForeignKey("patrons.id", ondelete="CASCADE"), index=True + ) + patron: Mapped[Patron] = relationship("Patron", back_populates="loan_checkouts") + + license_pool_id = Column(Integer, ForeignKey("licensepools.id"), index=True) + license_pool: Mapped[LicensePool] = relationship("LicensePool") + + timestamp = Column(DateTime(timezone=True), index=True) + + def __lt__(self, other): + return self.timestamp < other.timestamp + + class Hold(Base, LoanAndHoldMixin): """A patron is in line to check out a book.""" diff --git a/poetry.lock b/poetry.lock index d5b1b4aed..920695ab3 100644 --- a/poetry.lock +++ b/poetry.lock @@ -4152,6 +4152,17 @@ files = [ [package.dependencies] referencing = "*" +[[package]] +name = "types-openpyxl" +version = "3.1.0.20240205" +description = "Typing stubs for openpyxl" +optional = false +python-versions = ">=3.8" +files = [ + {file = "types-openpyxl-3.1.0.20240205.tar.gz", hash = "sha256:0b1c933f3a33e5f701a748426582aae360bc5c033ce7caea192687a1f99c7bd3"}, + {file = "types_openpyxl-3.1.0.20240205-py3-none-any.whl", hash = "sha256:734dfff2bfeeee0f46e78aed34f903130afe2364e9655c77b68436f187ec8a2d"}, +] + [[package]] name = "types-pillow" version = "10.2.0.20240111" @@ -4494,4 +4505,4 @@ lxml = ">=3.8" [metadata] lock-version = "2.0" python-versions = ">=3.10,<4" -content-hash = "09aa4cd1fa7881a2ace8a844ec797fcbc17f200353015d6f4ea5ed5ad44b32c8" +content-hash = "bcd28f5925d0af63b549c2ccda1a61527913a206f8ddb9854b5818349d4ad8de" diff --git a/pyproject.toml b/pyproject.toml index af6285150..7954a673e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -270,6 +270,7 @@ requests-mock = "1.11.0" types-aws-xray-sdk = "^2.11.0.13" types-Flask-Cors = "^4.0.0" types-jsonschema = "^4.17.0.5" +types-openpyxl = "^3.1.0.20240205" types-Pillow = "^10.0.0" types-psycopg2 = "^2.9.21" types-python-dateutil = "^2.8.19" diff --git a/tests/api/finland/test_ekirjasto.py b/tests/api/finland/test_ekirjasto.py index a97d3ec5c..9441a29df 100644 --- a/tests/api/finland/test_ekirjasto.py +++ b/tests/api/finland/test_ekirjasto.py @@ -238,7 +238,7 @@ def test_authentication_flow_document( controller_fixture.app.config["SERVER_NAME"] = "localhost" with controller_fixture.app.test_request_context("/"): - doc = provider._authentication_flow_document(None) + doc = provider._authentication_flow_document(controller_fixture.db.session) assert provider.label() == doc["description"] assert provider.flow_type == doc["type"] @@ -311,19 +311,12 @@ def test_secrets( ): provider = create_provider() - # Secrets are not set, so this will fail. - pytest.raises(InternalServerError, provider._check_secrets_or_throw) - assert provider.delegate_token_signing_secret == None - assert provider.delegate_token_encrypting_secret == None - provider.set_secrets(controller_fixture.db.session) - provider._check_secrets_or_throw() - - assert provider.delegate_token_signing_secret != None - assert provider.delegate_token_encrypting_secret != None + assert provider.delegate_token_signing_secret is not None + assert provider.delegate_token_encrypting_secret is not None - # Screts should be strong enough. + # Secrets should be strong enough. assert len(provider.delegate_token_signing_secret) > 30 assert len(provider.delegate_token_encrypting_secret) > 30 @@ -685,6 +678,9 @@ def test_authenticated_patron_delegate_token_expired( decoded_payload = provider.validate_ekirjasto_delegate_token( delegate_token, validate_expire=False ) + + assert type(decoded_payload) is dict + patron = provider.authenticated_patron( controller_fixture.db.session, decoded_payload ) @@ -718,6 +714,8 @@ def test_authenticated_patron_ekirjasto_token_invald( controller_fixture.db.session, patron ) + assert patron_delegate_id is not None + # Delegate token with the ekirjasto token. delegate_token = provider.create_ekirjasto_delegate_token( ekirjasto_token, patron_delegate_id, expires_at diff --git a/tests/api/finland/test_opensearch_analytics_search.py b/tests/api/finland/test_opensearch_analytics_search.py index a3150b1cc..7894162a1 100644 --- a/tests/api/finland/test_opensearch_analytics_search.py +++ b/tests/api/finland/test_opensearch_analytics_search.py @@ -8,7 +8,7 @@ class MockLibrary: - def short_name(): + def short_name(self): return "testlib" diff --git a/tests/api/test_circulationapi.py b/tests/api/test_circulationapi.py index 91f1d59c5..d354a1081 100644 --- a/tests/api/test_circulationapi.py +++ b/tests/api/test_circulationapi.py @@ -173,6 +173,51 @@ def test_borrow_sends_analytics_event(self, circulation_api: CirculationAPIFixtu loan, hold, is_new = self.borrow(circulation_api) assert 3 == circulation_api.analytics.count + # Finland + def test_borrow_is_added_to_checkout_history( + self, circulation_api: CirculationAPIFixture + ): + now = utc_now() + loaninfo = LoanInfo( + circulation_api.pool.collection, + circulation_api.pool.data_source, + circulation_api.pool.identifier.type, + circulation_api.pool.identifier.identifier, + now, + now + timedelta(seconds=3600), + external_identifier=circulation_api.db.fresh_str(), + ) + circulation_api.remote.queue_checkout(loaninfo) + now = utc_now() + + loan, hold, is_new = self.borrow(circulation_api) + + # A checkout history row was created + assert 1 == len(circulation_api.patron.loan_checkouts) + + # Try to 'borrow' the same book again. + circulation_api.remote.queue_checkout(AlreadyCheckedOut()) + loan, hold, is_new = self.borrow(circulation_api) + + assert loaninfo.external_identifier == loan.external_identifier + + # Since the loan already existed, no new history item was created. + assert 1 == len(circulation_api.patron.loan_checkouts) + + # Now try to renew the book. + circulation_api.remote.queue_checkout(loaninfo) + loan, hold, is_new = self.borrow(circulation_api) + + # Renewals are counted as checkouts + assert 2 == len(circulation_api.patron.loan_checkouts) + + # Loans of open-access books go through a different code + # path, but they count as loans nonetheless. + circulation_api.pool.open_access = True + circulation_api.remote.queue_checkout(loaninfo) + loan, hold, is_new = self.borrow(circulation_api) + assert 3 == len(circulation_api.patron.loan_checkouts) + def test_attempt_borrow_with_existing_remote_loan( self, circulation_api: CirculationAPIFixture ):