Skip to content

Commit 2ee8ba9

Browse files
authored
AAP-50130: Fallback authentication only on migrated users (#778)
## Description <!-- Mandatory: Provide a clear, concise description of the changes and their purpose --> - What is being changed? Modifies the controller fallback authentication so that it only is triggered if the gateway user has flag `use_controller_password=true`. Once the controller fallback authentication is completed, this flag is set to false to disallow users from then signing in again with their controller password. - Why is this change needed? This change is needed to ensure passwords can only be set from their controller passwords once. This provides more security benefits so that the controller password cannot always be used as a "fallback" password, limiting exposure if this password had ever been leaked after it has been changed. - How does this change address the issue? This change addresses the issue by adding another safeguard check in the fallback controller authentication logic to validate that this flag is set on the user, and if it is, it will update it to false, after the password has been modified. ## 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 Testing instructions use aap-dev for validation 1. Deploy AAP Dev, with this branch, and also this [gateway PR](ansible-automation-platform/aap-gateway#918) 2. Check `/gateway/v1/users/`, validate the administrator account has `use_controller_password=false` 3. Attempt to sign in via gateway with administrator controller password. This should fail. Only gateway password will work 4. Exec into controller-task container and run the following command to create a user on controller only - ```bash ANSIBLE_REVERSE_RESOURCE_SYNC=false awx-manage shell_plus --quiet-load -c "User.objects.create_user(username='dummy', email='[email protected]', password='test', first_name='dummy', last_name='user')" ``` 5. Exec into gateway-api pod and run the migrate_service_data command - ```bash aap-gateway-manage migrate_service_data --username=admin ``` 6. Check `/gateway/v1/users`, validate dummy account exists, with `use_controller_password=true` 7. Sign into gateway with the dummy user account, using the password created in controller, sign in should succeed. 8. Check `/gateway/v1/users`, see that, with `use_controller_password=false` 9. Change dummy user password in gateway 10. Attempt to sign in with dummy user again using original controller password. Signin should fail. Only gateway password can be used now. ### Expected Results Controller fallback authentication can only happen once for any account, and only on those accounts which were migrated via migrate_service_data ## 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 -->
1 parent 894af11 commit 2ee8ba9

File tree

4 files changed

+113
-64
lines changed

4 files changed

+113
-64
lines changed

ansible_base/authentication/authenticator_plugins/local.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,17 @@ def _can_authenticate_from_controller(self, username, password):
9494
If the user is valid, update the gateway users credentials with the controller credentials.
9595
"""
9696
try:
97-
UserModel._default_manager.get_by_natural_key(username)
97+
user = UserModel._default_manager.get_by_natural_key(username)
9898
except UserModel.DoesNotExist:
9999
logger.warning(f"User '{username}' does not exist in the database.")
100100
return False
101101

102+
# Skip controller authentication if user has use_controller_password field set to False
103+
# Default to False when field doesn't exist (test environments)
104+
if not getattr(user, 'use_controller_password', False):
105+
logger.warning(f"User '{username}' password not in Controller.")
106+
return False
107+
102108
if controller_user := self._get_controller_user(username, password):
103109
# Validate controller_user has a ldap_dn field, if it is not None, then the user is a local user
104110
ldap_dn = controller_user.get("ldap_dn")
@@ -174,7 +180,14 @@ def update_gateway_user(self, username, password):
174180
"""
175181
user = UserModel._default_manager.get_by_natural_key(username)
176182
user.set_password(password)
177-
user.save(update_fields=['password'])
183+
184+
# Set use_controller_password to False if the field exists
185+
update_fields = ['password']
186+
if hasattr(user, 'use_controller_password'):
187+
user.use_controller_password = False
188+
update_fields.append('use_controller_password')
189+
190+
user.save(update_fields=update_fields)
178191
logger.info(f"Updated user {username} gateway account")
179192

180193
def _convert_to_seconds(self, s):

ansible_base/authentication/utils/authentication.py

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,7 @@ def _handle_no_merge_strategy(uid: str, authenticator: Authenticator) -> str:
145145
if AuthenticatorUser.objects.filter(Q(uid=uid) | Q(user__username=uid)).exists():
146146
# Some other provider is providing this username so we need to create our own username
147147
new_username = get_local_username({'username': uid})
148-
logger.info(
149-
f'Authenticator {authenticator.name} wants to authenticate {uid} but that'
150-
f' username is already in use by another authenticator,'
151-
f' the user from this authenticator will be {new_username}'
152-
)
148+
logger.warning(f"User {uid} is already associated with an existing user, creating a new user with username {new_username}")
153149
return new_username
154150
else:
155151
# We didn't have an exact match but no other provider is servicing this uid so lets return that for usage
@@ -188,11 +184,7 @@ def _handle_email_fallback_strategy(uid: str, email: Union[str, list[str], None]
188184
# PROPOSAL FLOW: Does an AAP User with this email exist? No
189185
else:
190186
new_username = get_local_username({'username': uid})
191-
logger.warning(
192-
f"Authenticator {authenticator.name} wants to authenticate {uid}/{email} "
193-
"but that username is already in use by another authenticator, "
194-
f"the user from this authenticator will be {new_username}"
195-
)
187+
logger.warning(f"User {uid} is already associated with an existing user, creating a new user with username {new_username}")
196188
return new_username
197189

198190

@@ -219,10 +211,8 @@ def _handle_email_fallback_strategy(uid: str, email: Union[str, list[str], None]
219211
# else:
220212
# # Some other provider is providing this username so we need to create our own username
221213
# new_username = get_local_username({'username': uid})
222-
# logger.info(
223-
# f'Authenticator {authenticator.name} wants to authenticate {uid}/{email} but that'
224-
# f' username is already in use by another authenticator,'
225-
# f' the user from this authenticator will be {new_username}'
214+
# logger.warning(
215+
# f"User {uid} is already associated with an existing user, creating a new user with username {new_username}"
226216
# )
227217
# return new_username
228218

test_app/tests/authentication/authenticator_plugins/test_local.py

Lines changed: 90 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,25 @@ def mock_get_setting(setting_name):
2222
return None
2323

2424

25+
@pytest.fixture
26+
def controller_auth_mocks(user):
27+
"""
28+
Helper fixture that provides common mocks for controller authentication tests.
29+
Returns a context manager that sets up the basic mocks needed for controller auth.
30+
"""
31+
from contextlib import contextmanager
32+
33+
@contextmanager
34+
def mock_controller_auth():
35+
with (
36+
mock.patch.object(user, 'use_controller_password', True, create=True),
37+
mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user),
38+
):
39+
yield
40+
41+
return mock_controller_auth
42+
43+
2544
@mock.patch("rest_framework.views.APIView.authentication_classes", [SessionAuthentication])
2645
def test_local_auth_successful(unauthenticated_api_client, local_authenticator, user):
2746
"""
@@ -122,7 +141,7 @@ def test_can_authenticate_from_controller_nonexistent_user():
122141

123142

124143
@pytest.mark.django_db()
125-
def test_can_authenticate_from_controller_success(user):
144+
def test_can_authenticate_from_controller_success(user, controller_auth_mocks):
126145
"""
127146
Test that _can_authenticate_from_controller returns True when all conditions are met.
128147
"""
@@ -132,8 +151,8 @@ def test_can_authenticate_from_controller_success(user):
132151
user.password = "$encrypted$"
133152
user.save()
134153

135-
# Mock the controller user response - local user with encrypted password
136-
with mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": "$encrypted$"}):
154+
# Use fixture for common mocks and add specific controller user response
155+
with controller_auth_mocks(), mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": "$encrypted$"}):
137156
result = plugin._can_authenticate_from_controller(user.username, "password")
138157
assert result is True
139158

@@ -412,17 +431,20 @@ def test_can_authenticate_from_controller_logs_warning_invalid_format(user, expe
412431
"""
413432
plugin = AuthenticatorPlugin()
414433

415-
# Mock the request to return invalid format (non-dict result)
416-
with mock.patch('ansible_base.authentication.authenticator_plugins.local.get_setting', side_effect=mock_get_setting):
417-
with mock.patch('requests.get') as mock_get:
418-
mock_response = mock.Mock()
419-
mock_response.raise_for_status.return_value = None
420-
mock_response.json.return_value = {"count": 1, "results": ["invalid_string_not_dict"]}
421-
mock_get.return_value = mock_response
434+
# Mock use_controller_password to True to enable controller authentication
435+
mock_response = mock.Mock()
436+
mock_response.raise_for_status.return_value = None
437+
mock_response.json.return_value = {"count": 1, "results": ["invalid_string_not_dict"]}
422438

423-
with expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "user was not a dictionary"):
424-
result = plugin._can_authenticate_from_controller(user.username, "password")
425-
assert result is False
439+
with (
440+
mock.patch.object(user, 'use_controller_password', True, create=True),
441+
mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user),
442+
mock.patch('ansible_base.authentication.authenticator_plugins.local.get_setting', side_effect=mock_get_setting),
443+
mock.patch('requests.get', return_value=mock_response),
444+
expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "user was not a dictionary"),
445+
):
446+
result = plugin._can_authenticate_from_controller(user.username, "password")
447+
assert result is False
426448

427449

428450
@pytest.mark.django_db()
@@ -436,10 +458,15 @@ def test_can_authenticate_from_controller_logs_warning_not_local_user(user, expe
436458
user.password = "$encrypted$"
437459
user.save()
438460

439-
with mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "cn=user,dc=example,dc=com", "password": "$encrypted$"}):
440-
with expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an ldap user and can not be authenticated"):
441-
result = plugin._can_authenticate_from_controller(user.username, "password")
442-
assert result is False
461+
# Mock use_controller_password to True to enable controller authentication
462+
with (
463+
mock.patch.object(user, 'use_controller_password', True, create=True),
464+
mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user),
465+
mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "cn=user,dc=example,dc=com", "password": "$encrypted$"}),
466+
expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an ldap user and can not be authenticated"),
467+
):
468+
result = plugin._can_authenticate_from_controller(user.username, "password")
469+
assert result is False
443470

444471

445472
@pytest.mark.django_db()
@@ -629,23 +656,28 @@ def test_authenticate_successful_controller_validation_full_flow(user, local_aut
629656
user.password = "$encrypted$"
630657
user.save()
631658

632-
# Mock all the components for a successful flow
633-
with (
634-
mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": "$encrypted$"}),
635-
mock.patch('django.contrib.auth.backends.ModelBackend.authenticate') as mock_auth,
636-
):
637-
mock_auth.side_effect = [None, user] # First call fails, second succeeds after password update
659+
# Mock use_controller_password to True to enable controller authentication
660+
with mock.patch.object(user, 'use_controller_password', True, create=True):
661+
# Mock the database lookup to return our mocked user
662+
with mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user):
663+
# Mock all the components for a successful flow
664+
with (
665+
mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": "$encrypted$"}),
666+
mock.patch('django.contrib.auth.backends.ModelBackend.authenticate') as mock_auth,
667+
mock.patch.object(plugin, 'update_gateway_user') as mock_update,
668+
):
669+
mock_auth.side_effect = [None, user] # First call fails, second succeeds after password update
638670

639-
# Create request with gateway login path
640-
request = RequestFactory().get('/api/gateway/v1/login/')
641-
result = plugin.authenticate(request=request, username=user.username, password="password")
671+
# Create request with gateway login path
672+
request = RequestFactory().get('/api/gateway/v1/login/')
673+
result = plugin.authenticate(request=request, username=user.username, password="password")
642674

643-
# Verify successful authentication
644-
assert result is not None
645-
assert result == user
675+
# Verify successful authentication
676+
assert result is not None
677+
assert result == user
646678

647-
# Verify user was updated
648-
user.refresh_from_db()
679+
# Verify update_gateway_user was called
680+
mock_update.assert_called_once_with(user.username, "password")
649681

650682

651683
# Test connection and timeout errors
@@ -790,11 +822,15 @@ def test_can_authenticate_from_controller_enterprise_user(user, expected_log):
790822
"""
791823
plugin = AuthenticatorPlugin()
792824

793-
# Mock the controller user response with regular password (not encrypted)
794-
with mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": "regular_password"}):
795-
with expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an enterprise user and can not be authenticated"):
796-
result = plugin._can_authenticate_from_controller(user.username, "password")
797-
assert result is False
825+
# Mock use_controller_password to True to enable controller authentication
826+
with (
827+
mock.patch.object(user, 'use_controller_password', True, create=True),
828+
mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user),
829+
mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": "regular_password"}),
830+
expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an enterprise user and can not be authenticated"),
831+
):
832+
result = plugin._can_authenticate_from_controller(user.username, "password")
833+
assert result is False
798834

799835

800836
@pytest.mark.django_db()
@@ -804,11 +840,15 @@ def test_can_authenticate_from_controller_enterprise_user_missing_password(user,
804840
"""
805841
plugin = AuthenticatorPlugin()
806842

807-
# Mock the controller user response without password field
808-
with mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "username": "testuser"}):
809-
with expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an enterprise user and can not be authenticated"):
810-
result = plugin._can_authenticate_from_controller(user.username, "password")
811-
assert result is False
843+
# Mock use_controller_password to True to enable controller authentication
844+
with (
845+
mock.patch.object(user, 'use_controller_password', True, create=True),
846+
mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user),
847+
mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "username": "testuser"}),
848+
expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an enterprise user and can not be authenticated"),
849+
):
850+
result = plugin._can_authenticate_from_controller(user.username, "password")
851+
assert result is False
812852

813853

814854
@pytest.mark.django_db()
@@ -818,11 +858,15 @@ def test_can_authenticate_from_controller_enterprise_user_none_password(user, ex
818858
"""
819859
plugin = AuthenticatorPlugin()
820860

821-
# Mock the controller user response with None password
822-
with mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": None}):
823-
with expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an enterprise user and can not be authenticated"):
824-
result = plugin._can_authenticate_from_controller(user.username, "password")
825-
assert result is False
861+
# Mock use_controller_password to True to enable controller authentication
862+
with (
863+
mock.patch.object(user, 'use_controller_password', True, create=True),
864+
mock.patch('ansible_base.authentication.authenticator_plugins.local.UserModel._default_manager.get_by_natural_key', return_value=user),
865+
mock.patch.object(plugin, '_get_controller_user', return_value={"ldap_dn": "", "password": None}),
866+
expected_log('ansible_base.authentication.authenticator_plugins.local.logger', "warning", "is an enterprise user and can not be authenticated"),
867+
):
868+
result = plugin._can_authenticate_from_controller(user.username, "password")
869+
assert result is False
826870

827871

828872
# Test for timeout handling - if not timeout

test_app/tests/authentication/utils/test_authentication.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ def test_get_local_user_username_existing_user(self, random_user):
3131
[
3232
(None, 'is able to authenticate user', True),
3333
('local', 'already authenticated', True),
34-
('ldap', 'username is already in use by another authenticator', False),
34+
('ldap', 'is already associated with an existing user, creating a new user with username', False),
3535
('multiple', 'already authenticated', True),
3636
],
3737
)
@@ -43,7 +43,9 @@ def test_determine_username_from_uid(
4343
AuthenticatorUser.objects.create(uid=random_user.username, user=random_user, provider=local_authenticator)
4444
elif related_authenticator in ['ldap', 'multiple']:
4545
AuthenticatorUser.objects.create(uid=random_user.username, user=random_user, provider=ldap_authenticator)
46-
with expected_log(self.logger, 'info', info_message):
46+
# Use different log levels based on the scenario
47+
log_level = 'warning' if related_authenticator == 'ldap' else 'info'
48+
with expected_log(self.logger, log_level, info_message):
4749
new_username = authentication.determine_username_from_uid(uid, "", local_authenticator)
4850
if expected_username:
4951
assert new_username == random_user.username

0 commit comments

Comments
 (0)