Skip to content

Commit a46ebe5

Browse files
bhavenstBryan Havenstein
andauthored
AAP-36381: Make SAML username/permid required (#719)
## Description Some time back, attr_user_permanent_id and attr_username were made nullable/not-required since the docs make it seem like that is acceptable. It however is not and authentication fails without either of them. The docs have already been updated to state they are required, but the UI still shows them as optional. This just makes them required in the UI. ## Type of Change - [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 - [x] I have performed a self-review of my code - [ ] I have added relevant comments to complex code sections - [ ] I have updated documentation where needed - [ ] I have considered the security impact of these changes - [ ] I have considered performance implications - [ ] I have thought about error handling and edge cases - [ ] I have tested the changes in my local environment ## Testing Instructions ### Prerequisites ### Steps to Test 1. Try to create a SAML authenticator without specifying one or both of these params. 2. See that it's not accepted. 3. ### Expected Results Impossible to create SAML authenticator without username and permanent ID fields. ## Additional Context N/A --------- Co-authored-by: Bryan Havenstein <[email protected]>
1 parent 5144270 commit a46ebe5

File tree

2 files changed

+3
-11
lines changed
  • ansible_base/authentication/authenticator_plugins
  • test_app/tests/authentication/authenticator_plugins

2 files changed

+3
-11
lines changed

ansible_base/authentication/authenticator_plugins/saml.py

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,6 @@ class SAMLConfiguration(BaseAuthenticatorConfiguration):
128128
ui_field_label=_('User Email'),
129129
)
130130
IDP_ATTR_USERNAME = CharField(
131-
allow_null=True,
132-
required=False,
133131
help_text=_("The field in the assertion which represents the user's username."),
134132
ui_field_label=_('Username'),
135133
)
@@ -144,9 +142,7 @@ class SAMLConfiguration(BaseAuthenticatorConfiguration):
144142
ui_field_label=_('User First Name'),
145143
)
146144
IDP_ATTR_USER_PERMANENT_ID = CharField(
147-
allow_null=True,
148-
required=False,
149-
help_text=_("The field in the assertion which represents the user's permanent id (overrides IDP_ATTR_USERNAME)"),
145+
help_text=_("The field in the assertion which represents the user's permanent id."),
150146
ui_field_label=_('User Permanent ID'),
151147
)
152148
CALLBACK_URL = URLField(
@@ -193,10 +189,6 @@ def validate(self, attrs):
193189
except ValidationError as e:
194190
errors['SP_PRIVATE_KEY'] = e
195191

196-
idp_data = attrs.get('ENABLED_IDPS', {}).get(idp_string, {})
197-
if not idp_data.get('attr_user_permanent_id', None) and not idp_data.get('attr_username'):
198-
errors['IDP_ATTR_USERNAME'] = "Either IDP_ATTR_USERNAME or IDP_ATTR_USER_PERMANENT_ID needs to be set"
199-
200192
if errors:
201193
raise ValidationError(errors)
202194

test_app/tests/authentication/authenticator_plugins/test_saml.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ def test_saml_auth_successful(authenticate, unauthenticated_api_client, saml_aut
4040
pytest.param({"SP_PRIVATE_KEY": "really invalid"}, {"SP_PRIVATE_KEY": "Unable to load as PEM data"}, id="SP_PRIVATE_KEY is utterly invalid"),
4141
pytest.param(
4242
{"IDP_ATTR_USERNAME": None, "IDP_ATTR_USER_PERMANENT_ID": None},
43-
{"IDP_ATTR_USERNAME": "Either IDP_ATTR_USERNAME or IDP_ATTR_USER_PERMANENT_ID needs to be set"},
43+
{"IDP_ATTR_USERNAME": "This field may not be null.", "IDP_ATTR_USER_PERMANENT_ID": "This field may not be null."},
4444
id="null IDP_ATTR_USERNAME and IDP_ATTR_USER_PERMANENT_ID",
4545
),
4646
pytest.param(
@@ -55,7 +55,7 @@ def test_saml_auth_successful(authenticate, unauthenticated_api_client, saml_aut
5555
),
5656
pytest.param(
5757
{"-IDP_ATTR_USERNAME": None, "-IDP_ATTR_USER_PERMANENT_ID": None},
58-
{"IDP_ATTR_USERNAME": "Either IDP_ATTR_USERNAME or IDP_ATTR_USER_PERMANENT_ID needs to be set"},
58+
{"IDP_ATTR_USERNAME": "This field is required.", "IDP_ATTR_USER_PERMANENT_ID": "This field is required."},
5959
id="missing IDP_ATTR_USERNAME and IDP_ATTR_USER_PERMANENT_ID",
6060
),
6161
],

0 commit comments

Comments
 (0)