Skip to content

Commit daf39a8

Browse files
john-westcott-ivClaude (Anthropic)
andauthored
[AAP-53405] Expand how AzureAd searches for the USERNAME_FIELD (#838)
- Enhanced get_user_details method to search for USERNAME_FIELD across: * Original response data * ID token data (via user_data() method) * Standard user info from super().get_user_details() - Added comprehensive error handling with logging when USERNAME_FIELD not found - Improved data merging logic with proper precedence handling - Added deepcopy to ensure original response data immutability - Added comprehensive parameterized unit tests covering: * 5 different USERNAME_FIELD configurations * 3 error handling scenarios with missing fields * 4 token processing combinations (access_token, id_token, both, none) * 3 data merging behavior scenarios * 3 data immutability test cases - Total test coverage: 23 parameterized test scenarios ## Description <!-- Mandatory: Provide a clear, concise description of the changes and their purpose --> - What is being changed? The AzureAD will look in more places for the specified field to use for username. Additionally updated some of the help text. - Why is this change needed? Its a bit confusing for admins. - How does this change address the issue? It overlays a bunch of possible sources ad then checks in the results of all of them. ## Type of Change <!-- Mandatory: Check one or more boxes that apply --> - [ ] Bug fix (non-breaking change which fixes an issue) - [X] 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. Create an Azure AD authenticator leaving the Username field blank. 2. Log in as an AD user note your username, log out. 3. Log in as admin, delete the user your logged in as in step 2. 4. Alter the Azure AD authenticator to use a different field for the username (like email). 5. Log out as admin. 6. Log in as the AD user your email should now be your username. ### 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 (Anthropic) <[email protected]>
1 parent 401c3ba commit daf39a8

File tree

2 files changed

+452
-9
lines changed
  • ansible_base/authentication/authenticator_plugins
  • test_app/tests/authentication/authenticator_plugins

2 files changed

+452
-9
lines changed

ansible_base/authentication/authenticator_plugins/azuread.py

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import logging
2+
from copy import deepcopy
23

34
from django.utils.translation import gettext_lazy as _
45
from social_core.backends.azuread import AzureADOAuth2
@@ -26,15 +27,15 @@ class AzureADConfiguration(BaseAuthenticatorConfiguration):
2627
)
2728

2829
KEY = CharField(
29-
help_text=_("The OIDC key (Client ID) from your IDP. Will also be used as the 'audience' for JWT decoding."),
30+
help_text=_("The Client ID (OIDC Key) from Azure AD. Will also be used as the 'audience' for JWT decoding."),
3031
allow_null=False,
31-
ui_field_label=_('OIDC Key'),
32+
ui_field_label=_('Client ID'),
3233
)
3334

3435
SECRET = CharField(
35-
help_text=_("'The OIDC secret (Client Secret) from your IDP."),
36+
help_text=_("'The Client Secret (OIDC Secret) from Azure AD."),
3637
allow_null=True,
37-
ui_field_label=_('OIDC Secret'),
38+
ui_field_label=_('Secret'),
3839
)
3940

4041
GROUPS_CLAIM = CharField(
@@ -46,7 +47,11 @@ class AzureADConfiguration(BaseAuthenticatorConfiguration):
4647
)
4748

4849
USERNAME_FIELD = CharField(
49-
help_text=_("The name of the field from the assertion to use as the username. If not set will default to name"),
50+
help_text=_(
51+
"The name of the field to use as the username. "
52+
"This field can be in: the assertion, the id token or the standard user info. "
53+
"If not available this will default to the username."
54+
),
5055
required=False,
5156
allow_null=True,
5257
ui_field_label=_("Field to use as username"),
@@ -74,6 +79,25 @@ def get_user_details(self, response):
7479
This method is an override from social-core/social_core/backends/azuread.py
7580
It allows us to control what the username is.
7681
"""
82+
# start with the assertion we got back
83+
user_details = deepcopy(response)
84+
# At this point the response has not been overlaid with the info in the id token
85+
# so we need to get the info from the id token and overlay it
86+
if 'access_token' in response and 'id_token' in response:
87+
user_details.update(self.user_data(response['access_token'], response=response))
88+
# Finally overlay the fields from what we want to return initially
7789
return_object = super().get_user_details(response)
78-
return_object['username'] = response.get(self.setting("USERNAME_FIELD"), return_object['username'])
90+
user_details.update(return_object)
91+
92+
if self.setting("USERNAME_FIELD"):
93+
alternate_username = user_details.get(self.setting("USERNAME_FIELD"))
94+
if alternate_username:
95+
return_object['username'] = alternate_username
96+
else:
97+
valid_keys = list(user_details.keys())
98+
valid_keys.sort()
99+
logger.warning(
100+
f"Username field '{self.setting('USERNAME_FIELD')}' not found in AD response, using default username of {return_object['username']}."
101+
)
102+
logger.warning(f"Valid keys are: {valid_keys}")
79103
return return_object

0 commit comments

Comments
 (0)