Switch SAML auth to new general purpose filtering (PP-4422)

[tdilauro]

Important

Stacked PR — this branch is based on feature/general-purpose-filter (#3373). This PR should not be merged until that one lands.

Description

Replaces the custom python_expression_dsl package (a pyparsing-based custom DSL with its own AST, parser, and evaluator) with the new general-purpose FilterExpression utility introduced in the preceding PR. The SAMLSubjectFilter class is rewritten as a thin adapter over FilterExpression, and SAMLWebSSOAuthSettings.validate_filter_expression is updated to use FilterExpression.check_syntax() for syntax validation. The SAMLAuthenticationManager constructor now accepts subject_filter as an optional argument (defaulting to a plain SAMLSubjectFilter()), removing the need for callers to wire up DSL internals.

This is a pure internal refactor: patron-facing behavior and the filter expression syntax are unchanged.

Motivation and Context

The SAML patron-filtering feature used a hand-rolled pyparsing DSL (~1,100 lines across four files) that was complex and duplicated functionality now provided by the shared FilterExpression utility. Replacing it reduces maintenance and cognitive load and aligns SAML filtering with the common expression-evaluation approach coming to other parts of the codebase.

[Jira PP-4422]

How Has This Been Tested?

• Tests for the no-longer-needed components have been removed.
• Otherwise, the filter tests in this branch are mostly unchanged (except for imports and wiring).
• All tests and other checks pass locally.
• CI tests and checks pass.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

[claude]

Claude finished @tdilauro's task in 3m 55s —— View job

---

Code Review

• Gather context (read changed files)
• Review PR diff
• Analyze implementation correctness
• Check for bugs and issues
• Post findings

---

Summary

Clean refactor that replaces ~1,100 lines of hand-rolled pyparsing DSL with the shared FilterExpression utility. The mechanical substitution is sound: SAMLSubjectFilter is correctly reimplemented as a thin adapter, exception wrapping is preserved end-to-end, and the SAMLAuthenticationManager constructor defaulting to SAMLSubjectFilter() simplifies callers well. One intentional semantic change in the validation step deserves attention before merge.

Details

Minor: src/palace/manager/integration/patron_auth/saml/metadata/filter.py:62-78

The validate() method, and transitively SAMLWebSSOAuthSettings.validate_filter_expression, now performs syntax-only checking via ast.parse. The old implementation ran the full DSL parser, which required the expression to reference subject; an expression like attributes["x"].values[0] == "y" (no subject. prefix) would be rejected at configuration-save time. Under the new code it passes validation and only fails at evaluation time — meaning an admin who mistakenly saves such an expression will not get a validation error but will silently block all patron authentication until the expression is corrected.

The PR acknowledges this in the updated test:

# Expressions without "subject" will fail at evaluation time, not validation.
'attributes["eduPersonEntitlement"].values[0] == "urn:mace:nyu.edu:entl:lib:eresources"',
None,  # was: SAMLSubjectFilterError

The validate() docstring also notes the gap. The risk is bounded — only affects deliberately unusual expressions, and the old grammar's "must start with subject." check was an implicit side effect of the DSL's grammar rather than an explicit intent — but it is worth deciding consciously whether the weaker check is acceptable before merge.

circulation/src/palace/manager/integration/patron_auth/saml/metadata/filter.py

Lines 62 to 78 in 5c440a9

	def validate(self, expression: str) -> None:
	"""Validate the filter expression by checking its syntax.
	Note: only syntax is checked; names used in the expression are not
	verified against the evaluation context. An expression that references
	undefined names will pass this check but raise at evaluation time.
	:param expression: String containing the filter expression
	:raises SAMLSubjectFilterError: on any syntax error
	"""
	if not expression or not isinstance(expression, str):
	raise ValueError("Argument 'expression' must be a non-empty string")
	try:
	FilterExpression(expression).check_syntax()
	except FilterExpressionError as exc:
	raise SAMLSubjectFilterError(exc) from exc

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.32%. Comparing base (b6f5bcc) to head (5c440a9).

Files with missing lines	Patch %	Lines
...ntegration/patron_auth/saml/configuration/model.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                         @@
##           feature/general-purpose-filter    #3384      +/-   ##
==================================================================
- Coverage                           93.35%   93.32%   -0.04%     
==================================================================
  Files                                 508      501       -7     
  Lines                               46497    46003     -494     
  Branches                             6344     6307      -37     
==================================================================
- Hits                                43409    42930     -479     
+ Misses                               1999     1991       -8     
+ Partials                             1089     1082       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jonathangreen]

🎉 looks great!