Skip to content

Commit 819eaa1

Browse files
john-westcott-ivClaude (Assistant)Claude (Anthropic AI Assistant)
authored
[AAP-51570] Fix SAML security config validation error message formatting (#800)
This commit fixes a syntax error in the SAML authenticator plugin's validation error message for invalid security configuration keys. Changes: - Fixed string formatting syntax in ValidationError for SECURITY_CONFIG - Added comprehensive test case to validate the error message format - Test ensures proper error message: 'Invalid keys: key1, key2' The error occurred when users provided invalid SECURITY_CONFIG keys that aren't recognized by the underlying python-saml library. Now the validation properly catches these invalid keys and provides a clear, properly formatted error message. ## Description <!-- Mandatory: Provide a clear, concise description of the changes and their purpose --> - What is being changed? (see above) - Why is this change needed? The initial string was f formated but missing the f. However, the string was marked for translation and we can't do that with f strings so I altered it to use the % formatting notation. - How does this change address the issue? The string will now properly display the bad fields. ## Type of Change <!-- Mandatory: Check one or more boxes that apply --> - [X] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update - [ ] Test update - [ ] Refactoring (no functional changes) - [ ] Development environment change - [ ] Configuration change ## Self-Review Checklist <!-- These items help ensure quality - they complement our automated CI checks --> - [x] I have performed a self-review of my code - [x] I have added relevant comments to complex code sections - [x] I have updated documentation where needed - [x] I have considered the security impact of these changes - [x] I have considered performance implications - [x] I have thought about error handling and edge cases - [x] I have tested the changes in my local environment ## Testing Instructions <!-- Optional for test-only changes. Mandatory for all other changes --> <!-- Must be detailed enough for reviewers to reproduce --> ### Prerequisites <!-- List any specific setup required --> ### Steps to Test 1. See JIRA 2. 3. ### Expected Results <!-- Describe what should happen after following the steps --> ## Additional Context <!-- Optional but helpful information --> ### Required Actions <!-- Check if changes require work in other areas --> <!-- Remove section if no external actions needed --> - [ ] Requires documentation updates <!-- API docs, feature docs, deployment guides --> - [ ] Requires downstream repository changes <!-- Specify repos: django-ansible-base, eda-server, etc. --> - [ ] Requires infrastructure/deployment changes <!-- CI/CD, installer updates, new services --> - [ ] Requires coordination with other teams <!-- UI team, platform services, infrastructure --> - [ ] Blocked by PR/MR: #XXX <!-- Reference blocking PRs/MRs with brief context --> ### Screenshots/Logs <!-- Add if relevant to demonstrate the changes --> --------- Co-authored-by: Claude (Assistant) <[email protected]> Co-authored-by: Claude (Anthropic AI Assistant) <[email protected]>
1 parent e72923b commit 819eaa1

File tree

2 files changed

+103
-1
lines changed
  • ansible_base/authentication/authenticator_plugins
  • test_app/tests/authentication/authenticator_plugins

2 files changed

+103
-1
lines changed

ansible_base/authentication/authenticator_plugins/saml.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,12 @@ def validate(self, attrs):
208208
raise ValidationError(_("Failed to load config: %(e)s") % {"e": e})
209209

210210
if invalid_security_settings:
211-
raise ValidationError({'SECURITY_CONFIG': _("Invalid keys:) {', '.join(invalid_security_settings)}")})
211+
raise ValidationError(
212+
{
213+
'SECURITY_CONFIG': _("Invalid keys: %(keys)s, Valid keys: %(valid_keys)s")
214+
% {"keys": ', '.join(sorted(invalid_security_settings)), "valid_keys": ', '.join(sorted(valid_security_settings))}
215+
}
216+
)
212217

213218
response = super().validate(attrs)
214219
return response

test_app/tests/authentication/authenticator_plugins/test_saml.py

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,47 @@
44
import pytest
55
from django.conf import settings
66
from django.test.utils import override_settings
7+
from onelogin.saml2.settings import OneLogin_Saml2_Settings
8+
from social_core.backends.saml import SAMLAuth, SAMLIdentityProvider
79

810
from ansible_base.authentication.authenticator_plugins.saml import AuthenticatorPlugin
911
from ansible_base.authentication.session import SessionAuthentication
12+
from ansible_base.authentication.social_auth import AuthenticatorConfigTestStrategy, AuthenticatorStorage
1013
from ansible_base.lib.utils.encryption import ENCRYPTED_STRING
1114
from ansible_base.lib.utils.response import get_fully_qualified_url, get_relative_url
1215

1316
authenticated_test_page = "authenticator-list"
1417

18+
idp_string = 'IdP'
19+
20+
21+
def get_valid_saml_security_settings(saml_configuration):
22+
"""
23+
Helper function to get valid SAML security settings dynamically.
24+
This mirrors the logic in SAMLConfiguration.validate() method.
25+
"""
26+
# Create a minimal configuration for getting valid security settings
27+
attrs = saml_configuration.copy()
28+
attrs['ENABLED_IDPS'] = {
29+
idp_string: {
30+
'url': 'https://example.com/sso',
31+
'x509cert': attrs.get('IDP_X509_CERT', ''),
32+
'entity_id': 'https://example.com/entity',
33+
'attr_username': 'username',
34+
'attr_user_permanent_id': 'uid',
35+
}
36+
}
37+
38+
saml_auth = SAMLAuth(AuthenticatorConfigTestStrategy(AuthenticatorStorage(), additional_settings=attrs))
39+
saml_auth.redirect_uri = attrs['CALLBACK_URL']
40+
idp = SAMLIdentityProvider(idp_string, **attrs['ENABLED_IDPS'][idp_string])
41+
config = saml_auth.generate_saml_config(idp=idp)
42+
43+
settings = OneLogin_Saml2_Settings(settings=config)
44+
settings._security = {}
45+
settings._add_default_values()
46+
return sorted(settings._security.keys())
47+
1548

1649
@mock.patch("rest_framework.views.APIView.authentication_classes", [SessionAuthentication])
1750
@mock.patch("ansible_base.authentication.authenticator_plugins.saml.AuthenticatorPlugin.authenticate")
@@ -58,6 +91,26 @@ def test_saml_auth_successful(authenticate, unauthenticated_api_client, saml_aut
5891
{"IDP_ATTR_USERNAME": "This field is required.", "IDP_ATTR_USER_PERMANENT_ID": "This field is required."},
5992
id="missing IDP_ATTR_USERNAME and IDP_ATTR_USER_PERMANENT_ID",
6093
),
94+
pytest.param(
95+
{"SECURITY_CONFIG": {"invalidKey1": "value1", "invalidKey2": "value2"}},
96+
{
97+
"SECURITY_CONFIG": (
98+
"Invalid keys: invalidKey1, invalidKey2, "
99+
"Valid keys: allowRepeatAttributeName, authnRequestsSigned, digestAlgorithm, "
100+
"failOnAuthnContextMismatch, logoutRequestSigned, logoutResponseSigned, "
101+
"metadataCacheDuration, metadataValidUntil, nameIdEncrypted, rejectDeprecatedAlgorithm, "
102+
"requestedAuthnContext, requestedAuthnContextComparison, signMetadata, signatureAlgorithm, "
103+
"wantAssertionsEncrypted, wantAssertionsSigned, wantAttributeStatement, wantMessagesSigned, "
104+
"wantNameId, wantNameIdEncrypted"
105+
)
106+
},
107+
id="invalid security config keys",
108+
),
109+
pytest.param(
110+
{"SECURITY_CONFIG": {"zebra": "value1", "alpha": "value2", "beta": "value3"}},
111+
{"SECURITY_CONFIG": ("Invalid keys: alpha, beta, zebra")},
112+
id="invalid security config keys are sorted alphabetically",
113+
),
61114
],
62115
)
63116
def test_saml_create_authenticator_error_handling(
@@ -99,6 +152,50 @@ def test_saml_create_authenticator_error_handling(
99152
assert value in response.data[key]
100153

101154

155+
def test_saml_security_config_validation_with_dynamic_keys(admin_api_client, saml_configuration):
156+
"""
157+
Test that SAML security config validation uses dynamically obtained valid keys
158+
and that invalid keys are sorted alphabetically in error messages.
159+
"""
160+
# Get the valid security settings dynamically
161+
valid_keys = get_valid_saml_security_settings(saml_configuration)
162+
163+
# Test with invalid keys that should be sorted alphabetically
164+
invalid_keys = ["zebra", "alpha", "beta"]
165+
security_config = {key: f"value_{key}" for key in invalid_keys}
166+
167+
saml_configuration["SECURITY_CONFIG"] = security_config
168+
169+
url = get_relative_url("authenticator-list")
170+
data = {
171+
"name": "SAML authenticator with invalid security config",
172+
"enabled": True,
173+
"create_objects": True,
174+
"remove_users": True,
175+
"configuration": saml_configuration,
176+
"type": "ansible_base.authentication.authenticator_plugins.saml",
177+
}
178+
179+
response = admin_api_client.post(url, data=data, format="json")
180+
assert response.status_code == 400
181+
182+
# Verify the response contains security config errors
183+
assert "SECURITY_CONFIG" in response.data
184+
error_message = str(response.data["SECURITY_CONFIG"][0])
185+
186+
# Verify invalid keys are sorted alphabetically
187+
expected_invalid_keys = "alpha, beta, zebra"
188+
assert f"Invalid keys: {expected_invalid_keys}" in error_message
189+
190+
# Verify valid keys are included and sorted
191+
expected_valid_keys = ", ".join(valid_keys)
192+
assert f"Valid keys: {expected_valid_keys}" in error_message
193+
194+
# Verify that the valid keys we got dynamically match what's in the error message
195+
# This ensures our helper function works correctly
196+
assert expected_valid_keys in error_message
197+
198+
102199
def test_saml_create_authenticator_does_not_leak_private_key_on_error(
103200
admin_api_client,
104201
saml_configuration,

0 commit comments

Comments
 (0)