[AAP-39842] Fix: Validate email addresses from authenticators before persisting#972
Open
[AAP-39842] Fix: Validate email addresses from authenticators before persisting#972
Conversation
…sting Authenticators (SAML, LDAP, etc.) can provide invalid email addresses (e.g., UIDs without an @ sign) which bypass serializer validation and get stored directly on User objects. This causes 500 errors when downstream services (controller) reject the invalid email via their API. Add email format validation to normalize_and_get_email() using Django's built-in EmailValidator. Invalid emails are rejected with a warning log and the user is created with an empty email instead. Also validate the email parameter in get_or_create_authenticator_user() before it reaches the User model defaults. Debuggernaut autonomous fix. See PR description for full analysis. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNormalize and validate emails in authentication utilities; invalid emails are logged and coerced to empty. Persisted AuthenticatorUser and created User now use normalized email. Tests added for normalization behavior and logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/authentication/utils/authentication.py`:
- Around line 5-8: Reorder the imports so third-party libraries (e.g., Django,
social_core, etc.) appear before first-party ansible_base imports; specifically
ensure any imports from external packages come first and then import
get_authenticator_class, Authenticator, AuthenticatorUser, AuthenticatorStorage,
AuthenticatorStrategy, and normalize_and_get_email from ansible_base, preserving
the same symbols but changing their order to satisfy linting import grouping
rules.
In `@ansible_base/authentication/utils/user.py`:
- Around line 1-11: Reorder the imports in
ansible_base/authentication/utils/user.py to satisfy linting: place Django and
other third-party imports (e.g., django.contrib.auth.models.AbstractUser,
django.core.exceptions.ValidationError, django.core.validators.validate_email)
before the local/first-party ansible_base imports (e.g.,
ansible_base.authentication.authenticator_plugins.utils.get_authenticator_plugin,
ansible_base.authentication.models.AuthenticatorUser,
ansible_base.lib.utils.models.is_system_user); keep the logger definition
(logger) unchanged and ensure typing and logging imports remain appropriately
grouped.
In `@test_app/tests/authentication/utils/test_user.py`:
- Around line 1-5: Reorder the imports in the test module to satisfy isort:
place standard library imports first (unittest.mock), then third-party imports
(pytest), then local application imports (from
ansible_base.authentication.models import AuthenticatorUser and from
ansible_base.authentication.utils.user import can_user_change_password,
normalize_and_get_email); ensure grouping and alphabetical ordering according to
the project's isort configuration so the import block matches other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26d51652-6cb6-44ce-b56e-8e6f434a929b
📒 Files selected for processing (3)
ansible_base/authentication/utils/authentication.pyansible_base/authentication/utils/user.pytest_app/tests/authentication/utils/test_user.py
Separate third-party imports (django, social_core, pytest) from first-party imports (ansible_base) per isort black profile. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…r_user Remove unnecessary ternary (auth_user is always set by Step 4) and conditional log string to bring function under SonarCloud's limit of 15. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The test_gocau_auth_user_needs_creation test asserts on the 'attaching to existing user' log substring. Keep the ternary for the log but remove the unnecessary final_user guard instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Autonomous fix for AAP-39842: Authenticators can create users with invalid emails.
Authenticators (SAML, LDAP, etc.) can provide invalid email addresses (e.g., UIDs without an
@sign) which bypass serializer validation and get stored directly on User objects. This causes 500 errors when downstream services (controller) reject the invalid email via their API on the/meendpoint.Root Cause Analysis
The
normalize_and_get_email()function inansible_base/authentication/utils/user.pynormalizes email input (strips whitespace, lowercases) but does not validate that the email is actually valid. Two code paths allow invalid emails to be persisted:get_or_create_authenticator_user()copiesemailfromuser_detailsdirectly intoget_or_create()defaults without validation (line 345 ofauthentication.py).social_core.pipeline.user.user_detailsupdates user email directly from backend details, andcapture_oauth_email_pipelinecallsnormalize_and_get_emailwhich doesn't reject invalid formats.Evidence
ansible_base/authentication/utils/user.py:11-26—normalize_and_get_emailstrips/lowercases but never checks for@or email validityansible_base/authentication/utils/authentication.py:345—details["email"]passed straight toget_or_createdefaultsansible_base/authentication/utils/authentication.py:336—emailparam onAuthenticatorUseralso stored without validationChanges Made
ansible_base/authentication/utils/user.py_is_valid_email()helper using Django's built-inEmailValidatornormalize_and_get_email()to reject invalid emails (returnsNone) with a warning log message that includes the invalid address and guidance to check authenticator email attribute mappingansible_base/authentication/utils/authentication.pyemailparameter inget_or_create_authenticator_user()so invalid emails from authenticators are cleaned before reachingAuthenticatorUseruser_details["email"]before passing toUser.objects.get_or_create()defaultstest_app/tests/authentication/utils/test_user.py@, missing parts), empty inputs, non-string types, and list inputsRationale
Using Django's built-in
EmailValidatorrather than a simple@check because:@signs, invalid characters)The fix is applied at the
normalize_and_get_emaillevel (centralized) so both social auth and non-social auth paths benefit from a single change.Alternative Approaches Considered
EmailValidatorapproach is simpler and more surgical.Assumptions
Testing
Risk Assessment
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests