Skip to content

Commit 3a74234

Browse files
authored
[AAP-48066] Allow login from multiple active authenticators (#774)
## Description This PR is based off of the following [PoC](#751). [Jira](https://issues.redhat.com/browse/AAP-48066) - What is being changed? This PR allows for multiple authenticators to match to a given AAP user, based on the email as the unifying key. When attempting to sign in via any authenticator with email - `[email protected]`, we will sign you into the AAP user associated with that email, provided only 1 AAP user exists with that email address. - Why is this change needed? This change is needed to standardize our users, and provide clear expectations about how users are signed in. With this change, we will no longer have unique users for each authenticator, and will instead be able to link multiple authenticators to a given AAP user. - How does this change address the issue? This change addresses the issue by adjusting the logic used to match users to usernames as described in the [proposal](https://handbook.eng.ansible.com/proposals/0082-Handling-Sign-In-Via-Multiple-Authenticators). Note - This change allows users to log into existing AAP accounts if one associated with their email exists. However in this flow - 1. Local AAP account created 2. Sign in via LDAP The user will sign in successfully, but due to how django-auth-ldap works, it will automatically unset the users local password preventing them from then signing in via the local password. We may be able to circumvent this. In my testing so far I have not yet been able too, but upon thinking about it further, it may be best to avoid circumventing it, to avoid messing with how django-auth-ldap handles its security. ## 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 - [x] 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 --> - [ ] 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 <!-- 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 (using aap-dev) 1. `make configure-sources`, use this DAB PR 4. `make aap` 5. Configure multiple authenticators such as LDAP, GitHub, SAML, etc in your AAP environment 6. (Optional) Create a local user, with same email address another authenticator users, and sign in as local user. Sign in succeeds 7. Check the /api/gateway/v1/users/<ID> endpoint, we should see associated_authenticators updated with this authenticator reference 8. Sign in via another authenticator (such as github), you should be signed into the same AAP Account, and now associated_authenticators should have multiple entries. 9. (Optional) Keep signing in with other authenticators, you should continue to sign into the same AAP account as long as the same email address is provided by authenticator Next test - 1. Create a new local user, without an email 2. Sign in via local, see the user is created and linked to local authenticator 3. Sign in via authenticator, a new AAP user should be created (Since no email was available to match on) 4. Sign in via another authenticator (which provides email). We should link to the AAP account created by the step 3 authenticator. ### Expected Results User can sign into AAP using any authenticator, and expect to sign into their existing AAP account, so long as it is associated with a matching email. ## 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 df497e5 commit 3a74234

File tree

14 files changed

+597
-80
lines changed

14 files changed

+597
-80
lines changed

ansible_base/authentication/authenticator_plugins/ldap.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,13 @@ def authenticate(self, request, username=None, password=None, **kwargs) -> (obje
554554
# In unit testing there were cases where the function we are in was being called before get_or_build_user.
555555
# Its unclear if that was just a byproduct of mocking or a real scenario.
556556
# Since this call is idempotent we are just going to call it again to ensure the AuthenticatorUser is created for update_user_claims
557-
get_or_create_authenticator_user(username.lower(), self.database_instance, user_details={}, extra_data=user_from_ldap.ldap_user.attrs.data)
557+
get_or_create_authenticator_user(
558+
uid=username.lower(),
559+
email=user_from_ldap.email,
560+
authenticator=self.database_instance,
561+
user_details={},
562+
extra_data=user_from_ldap.ldap_user.attrs.data,
563+
)
558564
return update_user_claims(user_from_ldap, self.database_instance, users_groups)
559565
except Exception:
560566
logger.exception(f"Encountered an error authenticating to LDAP {self.database_instance.name}")
@@ -582,8 +588,9 @@ def get_or_build_user(self, username, ldap_user):
582588
This gets called by _LDAPUser to create the user in the database.
583589
"""
584590
user, _authenticator_user, created = get_or_create_authenticator_user(
585-
username.lower(),
586-
self.database_instance,
591+
uid=username.lower(),
592+
email=ldap_user.attrs.data.get(ldap_user.settings.USER_ATTR_MAP['email'], ""),
593+
authenticator=self.database_instance,
587594
user_details={
588595
"username": username,
589596
},

ansible_base/authentication/authenticator_plugins/local.py

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from requests.auth import HTTPBasicAuth
1010

1111
from ansible_base.authentication.authenticator_plugins.base import AbstractAuthenticatorPlugin, BaseAuthenticatorConfiguration
12-
from ansible_base.authentication.utils.authentication import determine_username_from_uid, get_or_create_authenticator_user
12+
from ansible_base.authentication.utils.authentication import get_or_create_authenticator_user
1313
from ansible_base.authentication.utils.claims import update_user_claims
1414
from ansible_base.lib.utils.settings import get_setting
1515

@@ -50,16 +50,6 @@ def authenticate(self, request, username=None, password=None, **kwargs):
5050
logger.info(f"Local authenticator {self.database_instance.name} is disabled, skipping")
5151
return None
5252

53-
# Determine the user name for this authenticator, we have to call this so that we can "attach" to a pre-created user
54-
new_username = determine_username_from_uid(username, self.database_instance)
55-
# However we can't really accept a different username because we are the local authenticator imagine if:
56-
# User "a" is from another authenticator and has an AuthenticatorUser
57-
# User "a" tried to login from local authenticator
58-
# The above function will return a username of "a<hash>"
59-
# We then try to do local authentication with the database from a different username that will not exist in the database, so it would never work
60-
if new_username != username:
61-
return None
62-
6353
user = super().authenticate(request, username, password, **kwargs)
6454
controller_login_results = None
6555
if (
@@ -84,7 +74,8 @@ def authenticate(self, request, username=None, password=None, **kwargs):
8474
# it has an AuthenticatorUser associated with it.
8575
if user:
8676
get_or_create_authenticator_user(
87-
username,
77+
uid=username,
78+
email=user.email,
8879
authenticator=self.database_instance,
8980
user_details={},
9081
extra_data={

ansible_base/authentication/authenticator_plugins/radius.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,8 @@ def authenticate(self, request, username=None, password=None, **kwargs):
8787
return None
8888

8989
user, _authenticator_user, _is_created = get_or_create_authenticator_user(
90-
username,
90+
uid=username,
91+
email="",
9192
authenticator=self.database_instance,
9293
user_details={},
9394
extra_data={"username": username},

ansible_base/authentication/authenticator_plugins/tacacs.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,9 @@ def authenticate(self, request, username=None, password=None, **kwargs):
119119

120120
if reply.valid:
121121
user, _authenticator_user, _created = get_or_create_authenticator_user(
122-
username,
123-
self.database_instance,
122+
uid=username,
123+
email="",
124+
authenticator=self.database_instance,
124125
user_details={},
125126
extra_data={'username': username},
126127
)

ansible_base/authentication/social_auth.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
from ansible_base.authentication.authenticator_plugins.utils import generate_authenticator_slug, get_authenticator_class, get_authenticator_plugins
1515
from ansible_base.authentication.models import Authenticator, AuthenticatorUser
16+
from ansible_base.authentication.utils.user import normalize_and_get_email
1617
from ansible_base.lib.utils.response import get_fully_qualified_url
1718

1819
logger = logging.getLogger('ansible_base.authentication.social_auth')
@@ -189,6 +190,41 @@ def validate(self, serializer, data):
189190
return data
190191

191192

193+
def capture_oauth_email_pipeline(*args, backend, details, **kwargs):
194+
"""
195+
Pipeline function to capture and store OAuth provider email in AuthenticatorUser.
196+
"""
197+
if not backend or not hasattr(backend, 'database_instance') or not backend.database_instance:
198+
logger.warning("No backend or database_instance found in OAuth email pipeline")
199+
return
200+
201+
user = kwargs.get('user')
202+
if not user:
203+
return
204+
205+
uid = kwargs.get('uid')
206+
if not uid:
207+
logger.warning("No uid found in OAuth email pipeline")
208+
return
209+
210+
email = normalize_and_get_email(details.get('email', ''))
211+
if not email:
212+
email = ''
213+
214+
logger.debug(f"Capturing OAuth email for user {user.username}: {email}")
215+
216+
# Get or update the AuthenticatorUser with the email
217+
try:
218+
from ansible_base.authentication.utils.authentication import get_or_create_authenticator_user
219+
220+
get_or_create_authenticator_user(
221+
uid=uid, email=email, authenticator=backend.database_instance, user_details=details, extra_data=kwargs.get('response', {})
222+
)
223+
logger.info(f"Stored OAuth email {email} for user {user.username} from {backend.database_instance.name}")
224+
except Exception as e:
225+
logger.warning(f"Failed to store OAuth email for user {user.username}: {e}")
226+
227+
192228
def create_user_claims_pipeline(*args, backend, response, **kwargs):
193229
from ansible_base.authentication.utils.claims import update_user_claims
194230

ansible_base/authentication/utils/authentication.py

Lines changed: 137 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import logging
2-
from typing import Optional, Tuple
2+
from typing import Optional, Tuple, Union
33

44
from django.conf import settings
55
from django.contrib.auth import get_user_model
@@ -12,9 +12,12 @@
1212
from ansible_base.authentication.authenticator_plugins.utils import get_authenticator_class
1313
from ansible_base.authentication.models import Authenticator, AuthenticatorUser
1414
from ansible_base.authentication.social_auth import AuthenticatorStorage, AuthenticatorStrategy
15+
from ansible_base.authentication.utils.user import normalize_and_get_email
1516

1617
logger = logging.getLogger('ansible_base.authentication.utils.authentication')
1718

19+
merge_strategy = "Email fallback"
20+
1821

1922
class FakeBackend:
2023
def setting(self, *args, **kwargs):
@@ -125,18 +128,105 @@ def determine_username_from_uid_social(**kwargs) -> dict:
125128
)
126129

127130
alt_uid = authenticator.get_alternative_uid(**kwargs)
131+
email = kwargs.get('details', {}).get('email', None)
128132

129133
if migrated_username := migrate_from_existing_authenticator(
130134
uid=kwargs.get("uid"), alt_uid=alt_uid, authenticator=authenticator.database_instance, preferred_username=selected_username
131135
):
132136
username = migrated_username
133137
else:
134-
username = determine_username_from_uid(selected_username, authenticator.database_instance)
138+
username = determine_username_from_uid(selected_username, email, authenticator.database_instance)
135139

136140
return {"username": username}
137141

138142

139-
def determine_username_from_uid(uid: str = None, authenticator: Authenticator = None, alt_uid: Optional[str] = None) -> str:
143+
def _handle_no_merge_strategy(uid: str, authenticator: Authenticator) -> str:
144+
"""Handles the merge strategy where users are unique for each authenticator."""
145+
if AuthenticatorUser.objects.filter(Q(uid=uid) | Q(user__username=uid)).exists():
146+
# Some other provider is providing this username so we need to create our own username
147+
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+
)
153+
return new_username
154+
else:
155+
# We didn't have an exact match but no other provider is servicing this uid so lets return that for usage
156+
logger.info(f"Authenticator {authenticator.name} is able to authenticate user {uid} as {uid}")
157+
return uid
158+
159+
160+
def _handle_email_fallback_strategy(uid: str, email: Union[str, list[str], None], authenticator: Authenticator) -> str:
161+
"""Handles the merge strategy where AAP users are matched by email."""
162+
###
163+
# AUTHENTICATION STRATEGY OVERVIEW
164+
# For more detailed information, see the following proposal
165+
# https://handbook.eng.ansible.com/proposals/0082-Handling-Sign-In-Via-Multiple-Authenticators#how
166+
#
167+
# The logic below maps to the proposal flowchart, indicated by comments starting with "PROPOSAL FLOW"
168+
###
169+
user_model = get_user_model()
170+
normalized_email = normalize_and_get_email(email)
171+
172+
# PROPOSAL FLOW: Authenticator No Email provided
173+
if not normalized_email:
174+
return _handle_no_merge_strategy(uid, authenticator) # Logic is identical to the no-merge strategy
175+
176+
# PROPOSAL FLOW: Authenticator provides email
177+
existing_user_match = user_model.objects.filter(email__iexact=normalized_email)
178+
user_count = existing_user_match.count()
179+
180+
# PROPOSAL FLOW: Are there multiple AAP users with this Email? Yes
181+
if user_count > 1:
182+
logger.warning(f"Found more than 1 user matching {uid}/{email}, unable to determine which user to merge with!")
183+
raise AuthException(f"Found more than 1 user matching {uid}/{email}, unable to determine which user to merge with!")
184+
# PROPOSAL FLOW: Are there multiple AAP users with this Email? No only 1
185+
elif user_count == 1:
186+
return existing_user_match.first().username
187+
# PROPOSAL FLOW: Does an AAP User with this email exist? No
188+
else:
189+
new_username = get_local_username({'username': uid})
190+
logger.warning(
191+
f"Authenticator {authenticator.name} wants to authenticate {uid}/{email} "
192+
"but that username is already in use by another authenticator, "
193+
f"the user from this authenticator will be {new_username}"
194+
)
195+
return new_username
196+
197+
198+
# def _handle_email_username_fallback_strategy(uid: str, email: Union[str, list[str], None], authenticator: Authenticator) -> str:
199+
# """Handles the unused 'Email and Username fallback' strategy."""
200+
# # This code is not used anywhere, but is kept here for reference, in case we later want to enable it.
201+
# normalized_email = normalize_and_get_email(email)
202+
203+
# # Note: The original code uses .get(), which will raise an exception if zero or multiple
204+
# # objects are found, and then tries to call .count() on the result, which will fail.
205+
# # The logic is preserved here as-is.
206+
# if not normalized_email:
207+
# existing_user_match = Authenticator.objects.get(user__username=uid)
208+
# else:
209+
# existing_user_match = Authenticator.objects.get(user__email=normalized_email)
210+
211+
# if existing_user_match.count() == 1:
212+
# existing_user = existing_user_match[0].user
213+
# new_username = existing_user.username
214+
# logger.info(f"Authenticator {authenticator.name} matched {uid}/{email} from provider {existing_user_match[0].provider.name}, combining users")
215+
# return new_username
216+
# elif existing_user_match.count() > 1:
217+
# raise AuthException(f"Found more than 1 user matching {uid}/{email}, unable to determine which user to merge with!")
218+
# else:
219+
# # Some other provider is providing this username so we need to create our own username
220+
# new_username = get_local_username({'username': uid})
221+
# logger.info(
222+
# f'Authenticator {authenticator.name} wants to authenticate {uid}/{email} but that'
223+
# f' username is already in use by another authenticator,'
224+
# f' the user from this authenticator will be {new_username}'
225+
# )
226+
# return new_username
227+
228+
229+
def determine_username_from_uid(uid: str = None, email: Union[str, list[str], None] = None, authenticator: Authenticator = None) -> str:
140230
"""
141231
Determine what the username for the User object will be from the given uid and authenticator
142232
This will take uid like "bella" and search for an AuthenticatorUser and return:
@@ -157,30 +247,43 @@ def determine_username_from_uid(uid: str = None, authenticator: Authenticator =
157247
raise
158248

159249
# If we have an AuthenticatorUser with the exact uid and provider than we have a match
160-
exact_match = AuthenticatorUser.objects.filter(uid=uid, provider=authenticator)
161-
if exact_match.count() == 1:
162-
new_username = exact_match[0].user.username
250+
if (exact_match := AuthenticatorUser.objects.filter(uid=uid, provider=authenticator)).exists():
251+
new_username = exact_match.first().user.username
163252
logger.info(f"Authenticator {authenticator.name} already authenticated {uid} as {new_username}")
164253
return new_username
165254

166-
# We didn't get an exact match. If any other provider is using this uid our id will be uid<hash>
167-
if AuthenticatorUser.objects.filter(Q(uid=uid) | Q(user__username=uid)).count() != 0:
168-
# Some other provider is providing this username so we need to create our own username
169-
new_username = get_local_username({'username': uid})
170-
logger.info(
171-
f'Authenticator {authenticator.name} wants to authenticate {uid} but that'
172-
f' username is already in use by another authenticator,'
173-
f' the user from this authenticator will be {new_username}'
174-
)
175-
return new_username
255+
logger.debug(f"Authentication merge strategy is {merge_strategy}")
176256

177-
# We didn't have an exact match but no other provider is servicing this uid so lets return that for usage
178-
logger.info(f"Authenticator {authenticator.name} is able to authenticate user {uid} as {uid}")
179-
return uid
257+
# Dispatch to the correct handler based on the merge strategy
258+
# if merge_strategy == "": # AAP 2.5 Merge Strategy
259+
# return _handle_no_merge_strategy(uid, authenticator)
260+
if merge_strategy == "Email fallback": # AAP 2.6 Default Merge Strategy
261+
return _handle_email_fallback_strategy(uid, email, authenticator)
262+
# elif merge_strategy == "Email and Username fallback": # In the future, we may want to enable this
263+
# return _handle_email_username_fallback_strategy(uid, email, authenticator)
264+
else:
265+
raise AuthException(f"Got an invalid merge_strategy {merge_strategy}")
266+
267+
268+
def _validate_username_for_new_user(username: str, authenticator: Authenticator):
269+
"""
270+
Prevents creating a user if the username is already tied to another authenticator.
271+
Returns True if the username is available, False otherwise.
272+
273+
Note: This assumes `merge_strategy` is available in the scope, as in the original code.
274+
"""
275+
if merge_strategy is None:
276+
if conflicting_user := AuthenticatorUser.objects.filter(user__username=username).first():
277+
logger.error(
278+
f'Authenticator {authenticator.name} attempted to create an AuthenticatorUser for {username}'
279+
f' but that id is already tied to authenticator {conflicting_user.provider.name}'
280+
)
281+
return False # Validation failed
282+
return True # Validation passed
180283

181284

182285
def get_or_create_authenticator_user(
183-
uid: str, authenticator: Authenticator, user_details: dict = dict, extra_data: dict = dict
286+
uid: str, email: str, authenticator: Authenticator, user_details: dict = dict, extra_data: dict = dict
184287
) -> Tuple[Optional[AbstractUser], Optional[AuthenticatorUser], Optional[bool]]:
185288
"""
186289
Create the user object in the database along with it's associated AuthenticatorUser class.
@@ -200,47 +303,45 @@ def get_or_create_authenticator_user(
200303
logger.warning(f"AuthException: {e}")
201304
raise
202305

306+
# Step 1: Determine the username, running the migration logic FIRST. This is the key fix.
203307
if migrated_username := migrate_from_existing_authenticator(uid=uid, alt_uid=None, authenticator=authenticator, preferred_username=uid):
204308
username = migrated_username
205309
else:
206-
username = determine_username_from_uid(uid, authenticator)
310+
username = determine_username_from_uid(uid, email, authenticator)
207311

312+
# Step 2: Now that the username is finalized, try to find the AuthenticatorUser.
313+
auth_user = None
208314
created = None
209315
try:
210-
# First see if we have an auth user and if so update it
211316
auth_user = AuthenticatorUser.objects.get(uid=uid, provider=authenticator)
212317
auth_user.extra_data = extra_data
318+
auth_user.email = email
213319
auth_user.save()
214320
created = False
215321
except AuthenticatorUser.DoesNotExist:
216-
# Ensure that this username is not already tied to another authenticator
217-
auth_user = AuthenticatorUser.objects.filter(user__username=username).first()
218-
if auth_user is not None:
219-
logger.error(
220-
f'Authenticator {authenticator.name} attempted to create an AuthenticatorUser for {username}'
221-
f' but that id is already tied to authenticator {auth_user.provider.name}'
222-
)
322+
# If the AuthenticatorUser doesn't exist, validate the username before creating a new one.
323+
if not _validate_username_for_new_user(username, authenticator):
223324
return None, None, None
224325

225-
# ensure the authenticator isn't trying to pass along a cheeky is_superuser in user_details
326+
# Step 3: Get or create the main User model instance.
226327
details = {k: user_details.get(k, "") for k in ["first_name", "last_name", "email"]}
227-
# Create or get our user object
228328
local_user, user_created = get_user_model().objects.get_or_create(username=username, defaults=details)
229329
if user_created:
230330
logger.info(f"Authenticator {authenticator.name} created User {username}")
231331

332+
# Step 4: If the AuthenticatorUser was not found in Step 2, create it now.
232333
if created is None:
233-
# Create or get the user object. The get shouldn't happen but just incase something snuck in on us
234334
auth_user, created = AuthenticatorUser.objects.get_or_create(
235335
user=local_user,
336+
email=email,
236337
uid=uid,
237338
provider=authenticator,
238339
defaults={'extra_data': extra_data},
239340
)
240341
if created:
241-
extra = ''
242-
if not user_created:
243-
extra = ' attaching to existing user'
342+
extra = ' attaching to existing user' if not user_created else ''
244343
logger.debug(f"Authenticator {authenticator.name} created AuthenticatorUser for {username}{extra}")
245344

246-
return local_user, auth_user, created
345+
# Ensure the returned user object is the one linked to the auth_user.
346+
final_user = auth_user.user if auth_user else local_user
347+
return final_user, auth_user, created

0 commit comments

Comments
 (0)