Skip to content

Include matching attribute in SAML patron ID extraction logging. #2950

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

Merged
tdilauro merged 2 commits into from
Jan 6, 2026
+106 −29
Merged

Include matching attribute in SAML patron ID extraction logging. #2950

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
c0377de
Include matching attribute in SAML patron ID extraction logging.
tdilauro Dec 26, 2025
2b11c64
A couple more test cases to improve coverage
tdilauro Dec 27, 2025
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
24 changes: 16 additions & 8 deletions src/palace/manager/integration/patron_auth/saml/metadata/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1350,20 +1350,24 @@ def extract(self, subject: SAMLSubject) -> str | None:
self._logger.info(f"Trying to extract a unique patron ID from {repr(subject)}")

patron_id = None
source_attribute = None

# Check for matching attributes if (1) we got any and
# (2) we've configured any to match.
if subject.attribute_statement:
for patron_id_attribute in self._patron_id_attributes:
if patron_id_attribute in subject.attribute_statement.attributes:
patron_id_attribute = subject.attribute_statement.attributes[
patron_id_attribute
for patron_id_attribute_name in self._patron_id_attributes:
if patron_id_attribute_name in subject.attribute_statement.attributes:
patron_id_attribute_obj = subject.attribute_statement.attributes[
patron_id_attribute_name
]

# NOTE: It takes the first value.
# TODO: Any reason not to handle multiple values here?
patron_id_candidate = patron_id_attribute.values[0]
patron_id_candidate = patron_id_attribute_obj.values[0]
patron_id = self._extract_patron_id(patron_id_candidate)

if patron_id is not None:
source_attribute = patron_id_attribute_name
break

# If we haven't found a patron id in the other attributes,
Expand All @@ -1377,15 +1381,19 @@ def extract(self, subject: SAMLSubject) -> str | None:
patron_id_candidate = subject.name_id.name_id
patron_id = self._extract_patron_id(patron_id_candidate)

if patron_id is not None:
source_attribute = "NameID"

# Log an appropriate message, depending on the outcome.
attribute_origin = f"from {repr(subject)}"
attribute_origin = repr(subject)
if patron_id is None:
self._logger.error(
f"Failed to extract a unique patron ID {attribute_origin}"
f"Failed to extract a unique patron ID from {attribute_origin}"
Copy link
Member

@jonathangreen jonathangreen Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very (very) minor suggestion to remove "a" to match the other log message on line 1395.

Suggested change
f"Failed to extract a unique patron ID from {attribute_origin}"
f"Failed to extract unique patron ID from {attribute_origin}"

Copy link
Contributor Author

@tdilauro tdilauro Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this on purpose. The sentences aren't parallel because I'm naming the ID on l. 1395 (though they would've been had I mentioned the ID parenthetically there).

)
else:
self._logger.info(
f"Extracted a unique patron ID '{patron_id}' {attribute_origin}"
f"Extracted unique patron ID '{patron_id}' from attribute '{source_attribute}' "
f"in {attribute_origin}"
)

return patron_id
111 changes: 90 additions & 21 deletions tests/manager/integration/patron_auth/saml/metadata/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,16 @@ def test_init_accepts_list_of_attributes(self):

class TestSAMLSubjectPatronIDExtractor:
@pytest.mark.parametrize(
"subject,expected_patron_id,use_name_id,patron_id_attributes,patron_id_regular_expression",
"subject,expected_patron_id,use_name_id,patron_id_attributes,patron_id_regular_expression,expected_source_attribute",
[
pytest.param(
SAMLSubject("http://idp.example.com", None, None),
None,
True,
None,
None,
id="subject_without_patron_id",
None,
id="no-patron-id",
),
pytest.param(
SAMLSubject(
Expand All @@ -80,7 +81,8 @@ class TestSAMLSubjectPatronIDExtractor:
True,
None,
None,
id="subject_with_eduPersonTargetedID_attribute",
"eduPersonUniqueId",
id="edupersonuniqueid-priority",
),
pytest.param(
SAMLSubject(
Expand All @@ -102,7 +104,8 @@ class TestSAMLSubjectPatronIDExtractor:
True,
None,
None,
id="subject_with_eduPersonUniqueId_attribute",
"eduPersonUniqueId",
id="edupersonuniqueid",
),
pytest.param(
SAMLSubject(
Expand All @@ -116,7 +119,8 @@ class TestSAMLSubjectPatronIDExtractor:
True,
None,
None,
id="subject_with_uid_attribute",
"uid",
id="uid",
),
pytest.param(
SAMLSubject(
Expand All @@ -135,7 +139,8 @@ class TestSAMLSubjectPatronIDExtractor:
True,
None,
None,
id="subject_with_name_id",
"NameID",
id="name-id",
),
pytest.param(
SAMLSubject(
Expand All @@ -154,7 +159,8 @@ class TestSAMLSubjectPatronIDExtractor:
False,
None,
None,
id="subject_with_switched_off_use_of_name_id",
None,
id="name-id-disabled",
),
pytest.param(
SAMLSubject(
Expand All @@ -180,7 +186,8 @@ class TestSAMLSubjectPatronIDExtractor:
False,
[SAMLAttributeType.uid.name],
None,
id="patron_id_attributes_matching_attributes_in_subject",
"uid",
id="uid-configured",
),
pytest.param(
SAMLSubject(
Expand Down Expand Up @@ -209,7 +216,8 @@ class TestSAMLSubjectPatronIDExtractor:
SAMLAttributeType.uid.name,
],
None,
id="patron_id_attributes_matching_second_saml_attribute",
"uid",
id="uid-second-match",
),
pytest.param(
SAMLSubject(
Expand All @@ -235,7 +243,8 @@ class TestSAMLSubjectPatronIDExtractor:
False,
[SAMLAttributeType.givenName.name],
None,
id="patron_id_attributes_not_matching_attributes_in_subject",
None,
id="no-match",
),
pytest.param(
SAMLSubject(
Expand All @@ -261,7 +270,35 @@ class TestSAMLSubjectPatronIDExtractor:
True,
[SAMLAttributeType.givenName.name],
None,
id="patron_id_attributes_not_matching_attributes_in_subject_and_using_name_id_instead",
"NameID",
id="no-match-fallback-name-id",
),
pytest.param(
SAMLSubject(
"http://idp.example.com",
None,
SAMLAttributeStatement(
[
SAMLAttribute(
name=SAMLAttributeType.eduPersonTargetedID.name,
values=["2"],
),
SAMLAttribute(
name=SAMLAttributeType.eduPersonUniqueId.name,
values=["3"],
),
SAMLAttribute(
name=SAMLAttributeType.uid.name, values=["4"]
),
]
),
),
None,
True,
[SAMLAttributeType.givenName.name],
None,
None,
id="no-match-no-nameid",
),
pytest.param(
SAMLSubject(
Expand Down Expand Up @@ -290,7 +327,8 @@ class TestSAMLSubjectPatronIDExtractor:
SAMLAttributeType.mail.name,
],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_ORG,
id="patron_id_regular_expression_matching_saml_subject",
"eduPersonPrincipalName",
id="regex-match",
),
pytest.param(
SAMLSubject(
Expand Down Expand Up @@ -320,7 +358,8 @@ class TestSAMLSubjectPatronIDExtractor:
SAMLAttributeType.mail.name,
],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_ORG,
id="patron_id_regular_expression_matching_second_saml_attribute",
"eduPersonPrincipalName",
id="regex-second-match",
),
pytest.param(
SAMLSubject(
Expand Down Expand Up @@ -349,7 +388,8 @@ class TestSAMLSubjectPatronIDExtractor:
SAMLAttributeType.mail.name,
],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_ORG,
id="unicode_patron_id_regular_expression_matching_saml_subject",
"eduPersonPrincipalName",
id="regex-unicode",
),
pytest.param(
SAMLSubject(
Expand Down Expand Up @@ -378,7 +418,28 @@ class TestSAMLSubjectPatronIDExtractor:
SAMLAttributeType.mail.name,
],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_COM,
id="patron_id_regular_expression_not_matching_saml_subject",
None,
id="regex-no-match",
),
pytest.param(
SAMLSubject(
"http://idp.example.com",
SAMLNameID(SAMLNameIDFormat.UNSPECIFIED.value, "", "", "1"),
SAMLAttributeStatement(
[
SAMLAttribute(
name=SAMLAttributeType.eduPersonPrincipalName.name,
values=["patron@university.org"],
),
]
),
),
None,
True,
[SAMLAttributeType.mail.name],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_COM,
None,
id="regex-no-match-fallback-to-nameid",
),
pytest.param(
SAMLSubject(
Expand Down Expand Up @@ -412,7 +473,8 @@ class TestSAMLSubjectPatronIDExtractor:
SAMLAttributeType.mail.name,
],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_COM,
id="patron_id_regular_expression_not_matching_saml_attributes_but_matching_name_id",
"NameID",
id="regex-fallback-name-id",
),
pytest.param(
SAMLSubject(
Expand All @@ -431,6 +493,7 @@ class TestSAMLSubjectPatronIDExtractor:
False,
[SAMLAttributeType.pairwiseId.name],
None,
"pairwiseId",
id="subject-with-pairwise-id-attribute",
),
pytest.param(
Expand All @@ -453,6 +516,7 @@ class TestSAMLSubjectPatronIDExtractor:
False,
[SAMLAttributeType.pairwiseId.name],
saml_strings.PATRON_ID_REGULAR_EXPRESSION_ORG,
"pairwiseId",
id="pairwise-id-with-regex-extraction",
),
],
Expand All @@ -464,6 +528,7 @@ def test(
use_name_id: bool,
patron_id_attributes: list[str] | None,
patron_id_regular_expression: re.Pattern | None,
expected_source_attribute: str | None,
caplog: pytest.LogCaptureFixture,
):
"""Make sure that SAMLSubjectUIDExtractor correctly extracts a unique patron ID from the SAML subject.
Expand All @@ -474,12 +539,16 @@ def test(
:param patron_id_attributes: List of SAML attribute names that are used by
SAMLSubjectUIDExtractor to search for a patron ID
:param patron_id_regular_expression: Regular expression used to extract a patron ID from SAML attributes
:param expected_source_attribute: Expected source attribute name in the log message
"""
expected_message = (
f"Extracted a unique patron ID '{expected_patron_id}'"
if expected_patron_id
else "Failed to extract a unique patron ID"
)
if expected_patron_id and expected_source_attribute:
expected_message = (
f"Extracted unique patron ID '{expected_patron_id}' "
f"from attribute '{expected_source_attribute}' in "
)
else:
expected_message = "Failed to extract a unique patron ID from "

expect_log_level = logging.INFO if expected_patron_id else logging.ERROR

# Arrange
Expand Down