[PP-3772] Add patron blocking rules per library#3090
Open
dbernstein wants to merge 10 commits intomainfrom
Open
[PP-3772] Add patron blocking rules per library#3090dbernstein wants to merge 10 commits intomainfrom
dbernstein wants to merge 10 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3090 +/- ##
==========================================
- Coverage 93.22% 93.22% -0.01%
==========================================
Files 491 493 +2
Lines 45364 45607 +243
Branches 6246 6275 +29
==========================================
+ Hits 42292 42517 +225
- Misses 1984 2001 +17
- Partials 1088 1089 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbernstein
force-pushed
the
feature/PP-3772-Add-patron-blocking-rules-per-library
branch
2 times, most recently
from
fcbea27 to
3a302fd
Compare
2 tasks
dbernstein
force-pushed
the
feature/PP-3772-Add-patron-blocking-rules-per-library
branch
3 times, most recently
from
e227145 to
40c4ef5
Compare
Extends SIP2LibrarySettings with an optional patron_blocking_rules field (list of PatronBlockingRule objects). Each rule has a required name, a required rule expression, and an optional patron-facing message. Validation is integrated into the existing SettingsValidationError / BaseSettings pattern: empty name, empty rule expression, and duplicate rule names within a library all surface as INVALID_CONFIGURATION_OPTION ProblemDetails with the array index in the message, consistent with how all other LibrarySettings validation errors are reported. The authentication hook stub overrides authenticate() in SIP2AuthenticationProvider. After the base class authenticates a patron successfully, check_patron_blocking_rules() is called. Any rule whose rule expression equals the literal "BLOCK" returns a BLOCKED_CREDENTIALS (HTTP 403) ProblemDetail, using the rule's message if provided. All other expressions are pass-throughs (stub for a future rule engine). Storage requires no migration: settings are already a JSON blob in IntegrationLibraryConfiguration.settings_dict, and the new field defaults to an empty list that is excluded from model_dump(), so existing library rows continue to load without change. No changes to PatronAuthServicesController or IntegrationSettingsController are needed; the field round-trips through the existing libraries JSON payload path automatically. 30 new tests cover: PatronBlockingRule model, SIP2LibrarySettings round-trip and all validation failure cases, check_patron_blocking_rules pure-function behaviour, and the authenticate() override (blocked, unblocked, no rules, None pass-through, ProblemDetail pass-through). Non-SIP2 providers are confirmed unaffected.
MissingPlaceholderError now accepts an available dict and embeds all available keys + values in the message (e.g. "Available fields: fee_amount='$5.00', fines=5.0, ..."). The available attribute is also accessible for programmatic use.
* build_names now passes the full values mapping to MissingPlaceholderError.
* Both validate_rule_expression and evaluate_rule_expression_strict_bool now catch FunctionNotDefined separately and include the list of allowed functions in the error (e.g. "Allowed functions: age_in_years").
* Added _format_available_keys and _format_allowed_functions helpers. Imported FunctionNotDefined from simpleeval.
* patron_blocking.py — Removed RULE_VALIDATION_TEST_VALUES; build_values_from_sip2_info now returns all raw SIP2 fields verbatim plus the normalised fines float — the old explicit patron_type and dob mappings are gone since those fields appear naturally in the raw dict.
* basic.py — Pydantic static validator now only enforces structural constraints (empty name, empty rule, duplicate names, rule length ≤ 1000, message length ≤ 1000). The syntax/semantic check against live values is done exclusively in library_integration_validation.
* Tests — Updated test_rule_engine.py to assert available keys appear in missing-placeholder errors and allowed functions appear in function-not-found errors. Updated test_patron_blocking.py to remove TestRuleValidationTestValues, drop tests that relied on static Pydantic expression validation, and rewrite TestBuildValuesFromSip2Info to verify all raw fields pass through. Updated test_patron_auth.py to use {custom_field} instead of {dob} in the missing-placeholder test.
* check_patron_blocking_rules (the legacy "BLOCK" string sentinel) was deleted from patron_blocking.py along with its entire docblock. * TestCheckPatronBlockingRules (and the check_patron_blocking_rules import) were removed from test_patron_blocking.py. * Runtime auth flow wired up to the full SIP2 response * basic.py — added the _build_blocking_rule_values(patron, credentials) hook method. The base implementation calls build_runtime_values_from_patron (DB-backed fallback for any future non-SIP2 provider). authenticate() was updated to use the hook and also short-circuits when patron_blocking_rules is empty (avoids calling _build_blocking_rule_values unnecessarily). * sip2/provider.py — thread-local module variable _sip2_thread_local was added. remote_authenticate() now caches the raw SIP2 patron_information dict in _sip2_thread_local.last_info immediately after the network call, at zero extra cost. _build_blocking_rule_values() reads that cache and calls build_values_from_sip2_info(info) to expose every SIP2 field to rule expressions. If the cache is None (e.g. when _do_authenticate is patched in tests), it falls back to the patron-model path. Tests * TestSIP2AuthenticateWithBlockingRules received: * An autouse fixture that zeroes _sip2_thread_local.last_info before and after every test, preventing cross-test pollution. * Updated test_missing_placeholder_fails_closed — comment now correctly describes the patron-fallback path taken when remote_authenticate is bypassed. * Four new tests that directly set _sip2_thread_local.last_info to simulate the real runtime SIP2 path: * test_raw_sip2_field_blocks_when_rule_matches / …_allows_when_… * test_fines_normalised_from_sip2_fee_amount * test_sip2_info_takes_precedence_over_patron_model
…rocess. Also fixes poetry lock file merge conflict.
dbernstein
force-pushed
the
feature/PP-3772-Add-patron-blocking-rules-per-library
branch
from
40c4ef5 to
98d9c5e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds per-library patron blocking rules to the SIP2 authentication provider, with the rule model and evaluation hook placed at the
BasicAuthenticationProviderlevel for future reuse across other providers. Implements a simpleeval-based rule engine for patron blocking rules and wires it into SIP2 validation and runtime authentication.What this does
PatronBlockingRule— a frozen Pydantic value object withname(required),rule(required), andmessage(optional). Stored in the existing per-library settings JSON blob; no migration needed.patron_blocking_rulesfield onBasicAuthProviderLibrarySettings— all basic-auth providers inherit the field (hidden in the admin UI for now). Validation enforces non-emptyname/ruleand unique names within a library's rule list, with array-indexed error messages consistent with the existingProblemDetailvalidation pattern.check_patron_blocking_rules()function in a new protocol-agnostic module (palace/manager/integration/patron_auth/patron_blocking.py). v1 stub: any rule whoseruleexpression equals the literal string"BLOCK"returns aBLOCKED_CREDENTIALSProblemDetail (HTTP 403); all other expressions are no-ops.supports_patron_blocking_rules: ClassVar[bool]onBasicAuthenticationProvider— defaults toFalse. Providers set it toTrueto opt in to the blocking hook. Currently onlySIP2AuthenticationProvideropts in; Millenium, Kansas, and SirsiDynix can be enabled in follow-up PRs.BasicAuthenticationProvider.authenticaterefactor — the original logic is extracted into_do_authenticate. The publicauthenticatewraps it: ifsupports_patron_blocking_rulesisTrueand the ILS returns aPatron, blocking rules are evaluated before the patron is returned.New module:
rule_engine.pysrc/palace/manager/api/authentication/patron_blocking_rules/rule_engine.py{key}placeholders are compiled to safe variable identifiers (__v_key) and resolved via simpleeval'snamesdict — no string interpolation.bool; any other type is an error.evaluate_rule_expression_strict_boolraisesRuleEvaluationErroron missing placeholders, parse/eval errors, or non-bool results.validate_rule_expressiondoes a trial evaluation against a supplied test-values dict; rejects empty/too-long/invalid/non-bool expressions viaRuleValidationError.EvalWithCompoundTypeswith an explicit whitelist. First function:age_in_years(date_str, fmt=None, *, today=None)— ISO parse with dateutil fallback, injectabletodayfor deterministic tests.RuleValidationError,RuleEvaluationError(withrule_name),MissingPlaceholderError(withkey).MAX_RULE_LENGTH = 1000,MAX_MESSAGE_LENGTH = 1000.Updated:
patron_blocking.pyRULE_VALIDATION_TEST_VALUES— deterministic dict (fines,patron_type,dob) used only at admin-save validation time.build_runtime_values_from_patron(patron)— builds the runtime values dict from aPatronDB object (finesas float,patron_typeas str).dobis intentionally omitted for now; rules referencing it fail-closed.check_patron_blocking_rules_with_evaluator(rules, values, log=None)— evaluates rules via simpleeval, fail-closed on any error. Creates a fresh evaluator per call (thread-safe). Logs internal errors server-side without leaking them to the patron.check_patron_blocking_rules(literal-BLOCK sentinel) retained for its own unit tests.Updated:
basic.pyvalidate_patron_blocking_rulesfield validator — extended to callvalidate_rule_expressionagainstRULE_VALIDATION_TEST_VALUESfor each rule, and checklen(rule.message) > 1000. Errors surface asSettingsValidationErrorwithINVALID_CONFIGURATION_OPTIONand an index-prefixed message, matching existing admin field-error patterns.authenticatemethod — replaced the legacy literal-BLOCK check withbuild_runtime_values_from_patron+check_patron_blocking_rules_with_evaluator, passingself.logfor server-side diagnostics.Files changed
src/palace/manager/api/authentication/basic.py— Addedpatron_blocking_rulesfield + validator,supports_patron_blocking_rulesflag, andauthenticate/_do_authenticaterefactorsrc/palace/manager/integration/patron_auth/patron_blocking.py— New:PatronBlockingRulemodel andcheck_patron_blocking_rules()src/palace/manager/integration/patron_auth/sip2/provider.py— Setssupports_patron_blocking_rules = Truetests/manager/integration/patron_auth/test_patron_blocking.py— New: 29 unit tests covering the model, validation, pure function, and the flag/hook behaviortests/manager/integration/patron_auth/sip2/test_provider.py— SIP2 integration tests for the blocking hook through a real provider instanceNot in scope (v1)
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-3772
How Has This Been Tested?
New unit test coverage.
It was also manually tested locally with the front end changes (ThePalaceProject/circulation-admin#201)
Checklist