Fix login error messages and email verification UI (#4056)Fix login error messages and email verification UI #5362
Conversation
- Add CustomLoginForm with clear error messages for invalid credentials - Update login template to display form errors prominently with red styling - Fix verification_sent.html to remove 'signup' references per issue OWASP-BLT#4056 - Add comprehensive test suite for login functionality: * Standalone tests for core login logic and security * Django unit tests for form validation and authentication * Integration tests for full login flow and edge cases - Improve email confirmation message template - Configure custom login form in settings Fixes OWASP-BLT#4056
|
👋 Hi @angeliaaju! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
|
Warning Rate limit exceeded@angeliaaju has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughComprehensive login error handling overhaul introducing custom form validation with unified error messages, case-insensitive email authentication, enhanced account adapter for improved messaging, updated templates with better error display and email verification UI fixes, and extensive test coverage validating login scenarios, security protections, and user experience behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
📊 Monthly LeaderboardHi @angeliaaju! Here's how you rank for December 2025:
Leaderboard based on contributions in December 2025. Keep up the great work! 🚀 |
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
website/templates/account/login.html
Outdated
| <form class="flex flex-col space-y-4 login" | ||
| method="post" | ||
| action="{% url 'account_login' %}?next={{ request.path }}"> | ||
| {% csrf_token %} | ||
| {% if redirect_field_value %} | ||
| <input type="hidden" | ||
| name="{{ redirect_field_name }}" | ||
| value="{{ redirect_field_value }}" /> | ||
| {% endif %} |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
website/account_adapters.py
Outdated
| """ | ||
| Override to customize email confirmation behavior. | ||
| """ | ||
| super().send_confirmation_mail(request, emailconfirmation, signup) | ||
|
|
||
| # Add a user-friendly message | ||
| if signup: | ||
| messages.success( | ||
| request, | ||
| "Account created successfully! Please check your email to verify your account before signing in." | ||
| ) No newline at end of file |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
website/forms.py (1)
39-47: Consider preserving the original error code for programmatic handling.The
clean()override catches allValidationErrorexceptions and re-raises with a generic message. While this is good for user-facing security (prevents enumeration), it discards the original error code which could be useful for programmatic handling or logging.🔎 Proposed enhancement to preserve error code
def clean(self): """Override clean to provide better error messages.""" try: return super().clean() except forms.ValidationError: # Provide a cleaner error message raise forms.ValidationError( - "Invalid username/email or password. Please check your credentials and try again." + "Invalid username/email or password. Please check your credentials and try again.", + code="invalid_credentials" )LOGIN_TESTS_README.md (1)
140-146: Consider updating prerequisites to match actual test requirements.The pip install command includes packages like
sentry-sdkthat aren't strictly required for running login tests. Consider updating to only list truly required packages, or reference the project's requirements file instead.🔎 Suggested update
### Prerequisites ```bash # Install required dependencies -pip install django django-allauth dj-database-url django-environ sentry-sdk +pip install -r requirements.txt # Or use Poetry (if available) poetry install</details> </blockquote></details> <details> <summary>test_login_standalone.py (2)</summary><blockquote> `16-17`: **Path manipulation may not work as expected.** The path manipulation adds the parent of the parent directory to `sys.path`, but this file is at the repository root. This line appears to be a leftover from a different location or may cause import issues. <details> <summary>🔎 Suggested fix</summary> ```diff # Add the project root to Python path -sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
195-221: Move imports to module level for efficiency.The
timeanduuidmodules are imported inside methods, which adds overhead on each call. Move these to the module level.🔎 Suggested refactor
import os import sys +import time +import uuid from unittest.mock import patch, MagicMock # ... later in RateLimiter class: def is_rate_limited(self, ip_address): - import time current_time = time.time() # ... def record_attempt(self, ip_address): - import time # ... def create_session(self, user_id, remember_me=False): - import uuid - import time # ... def validate_session(self, session_id): - import time # ...website/account_adapters.py (1)
28-32: Consider using Django settings for the redirect URL.The hardcoded
"/"works but could be made more configurable by using Django'sLOGIN_REDIRECT_URLsetting or a custom setting.🔎 Suggested improvement
def get_email_confirmation_redirect_url(self, request): """ Redirect after email confirmation to a user-friendly page. """ - return resolve_url("/") + from django.conf import settings + return resolve_url(getattr(settings, 'EMAIL_CONFIRMATION_REDIRECT_URL', '/'))website/tests/test_login_functionality.py (4)
31-44: Theauthenticate()call doesn't verify the actual login session.Lines 42-44 call
authenticate()independently, which only verifies credentials are valid but doesn't confirm the test client's session was established. For stronger assertions, check the session directly like intest_login_integration.py.🔎 Suggested improvement
def test_valid_login_with_username(self): """Test successful login with valid username and password""" response = self.client.post(self.login_url, { 'login': 'testuser', 'password': 'testpass123' }) # Should redirect after successful login self.assertEqual(response.status_code, 302) - # User should be authenticated - user = authenticate(username='testuser', password='testpass123') - self.assertIsNotNone(user) - self.assertEqual(user.username, 'testuser') + # User should be authenticated in session + self.assertTrue('_auth_user_id' in self.client.session) + self.assertEqual(int(self.client.session['_auth_user_id']), self.test_user.id)
263-282: Rate limiting test doesn't meaningfully verify rate limiting behavior.This test accepts both 200 and 302 status codes (line 282), which means it passes regardless of whether rate limiting is actually enforced. The integration test uses
@override_settings(ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3)to configure rate limiting - consider doing the same here or documenting this as a smoke test only.🔎 Suggested improvement
+ @override_settings(ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3) def test_rate_limiting_protection(self): - """Test protection against brute force attacks""" + """Test protection against brute force attacks (smoke test)""" # Make multiple failed login attempts - for i in range(10): + for i in range(5): response = self.client.post(self.login_url, { 'login': 'testuser', 'password': 'wrongpassword' }) # All should return 200 (login page) but with error self.assertEqual(response.status_code, 200) - # The system should still allow valid login + # After rate limit, valid login may be blocked or allowed depending on implementation response = self.client.post(self.login_url, { 'login': 'testuser', 'password': 'testpass123' }) - # Should still work (basic test - actual rate limiting might be implemented differently) - self.assertIn(response.status_code, [200, 302]) + # Note: This test verifies the system handles multiple attempts gracefully + # Actual rate limiting verification requires checking for rate limit messages + self.assertIn(response.status_code, [200, 302, 429])Don't forget to add the import at the top:
from django.test import Client, TestCase, override_settings
218-234: Test doesn't verify redirect to the original protected page.The test accesses a protected URL first, then logs in via a separate POST without preserving the
nextparameter. This doesn't verify that the user is redirected back to the original protected page after login. The integration test (test_login_redirect_next_parameter) handles this better.🔎 Suggested improvement
def test_login_redirect_after_success(self): """Test redirect after successful login""" # Try to access protected page first protected_url = reverse('user') response = self.client.get(protected_url) # Should redirect to login with next parameter self.assertEqual(response.status_code, 302) + self.assertIn('next=', response.url) - # Now login - response = self.client.post(self.login_url, { + # Now login with the next parameter preserved + login_with_next = f"{self.login_url}?next={protected_url}" + response = self.client.post(login_with_next, { 'login': 'testuser', 'password': 'testpass123' - }, follow=True) + }) - # Should be redirected to home page or dashboard - self.assertEqual(response.status_code, 200) + # Should redirect to the protected URL + self.assertEqual(response.status_code, 302) + self.assertIn(protected_url, response.url)
8-10: Significant test duplication withtest_login_integration.py.Both test files cover overlapping scenarios (valid/invalid login, empty credentials, CSRF, SQL injection, XSS, rate limiting, remember me). Consider either:
- Consolidating into a single comprehensive test file
- Documenting distinct purposes (e.g., unit vs integration focus)
The integration tests include stronger session assertions (
_auth_user_idchecks) and propertearDowncleanup.website/tests/test_login_integration.py (4)
11-18: Remove unused imports.
ValidationError(line 15) andtime(line 18) are imported but never used in this file.🔎 Proposed fix
from django.contrib.auth import authenticate from django.contrib.auth.models import User from django.test import Client, TestCase, override_settings from django.urls import reverse -from django.core.exceptions import ValidationError from django.contrib.sessions.models import Session from allauth.account.models import EmailAddress -import time
307-327: Rate limiting test could better verify rate limiting is enforced.The test configures
ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3but makes 5 failed attempts without checking if rate limiting actually kicks in. If allauth's rate limiting is enabled, attempts 4-5 should be blocked or show a rate limit message.🔎 Suggested improvement to verify rate limiting
@override_settings(ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3) def test_rate_limiting_simulation(self): - """Test rate limiting behavior (if implemented)""" + """Test rate limiting behavior with ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3""" # Make multiple failed login attempts for i in range(5): response = self.client.post(self.login_url, { 'login': 'activeuser', 'password': 'wrongpassword' }) # Should stay on login page self.assertEqual(response.status_code, 200) + + # After exceeding limit, check for rate limit indication + if i >= 3: + # Verify rate limiting message or behavior + pass # Implementation-specific check needed # Valid login should still work (basic test) response = self.client.post(self.login_url, { 'login': 'activeuser', 'password': 'securepass123' }) - # Should either work or show rate limit message + # After rate limiting, valid login may be blocked self.assertIn(response.status_code, [200, 302])
396-405: Consider removing redundanttearDowncleanup.Django's
TestCasewraps each test in a database transaction that's rolled back automatically. This explicit cleanup is redundant and could cause issues with parallel test execution. If there's a specific reason for explicit cleanup (e.g., non-transactional database features), document it.🔎 Suggested simplification
- def tearDown(self): - """Clean up after tests""" - # Clear any remaining sessions - Session.objects.all().delete() - - # Clear users - User.objects.all().delete() - - # Clear email addresses - EmailAddress.objects.all().delete()
245-258: Consider usingreverse()for the protected URL.Line 248 uses a hardcoded URL
/dashboard/user/. For consistency with other tests and to catch URL changes, consider usingreverse('user')(as done intest_login_functionality.pyline 221).🔎 Suggested improvement
def test_login_redirect_next_parameter(self): """Test redirect to next parameter after login""" # Try to access protected page - protected_url = '/dashboard/user/' + protected_url = reverse('user') next_url = f"{self.login_url}?next={protected_url}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
LOGIN_TESTS_README.mdblt/settings.pytest_login_standalone.pywebsite/account_adapters.pywebsite/forms.pywebsite/templates/account/email/email_confirmation_message.txtwebsite/templates/account/email/email_confirmation_subject.txtwebsite/templates/account/login.htmlwebsite/templates/account/verification_sent.htmlwebsite/tests/test_login_functionality.pywebsite/tests/test_login_integration.py
🧰 Additional context used
🧬 Code graph analysis (2)
website/tests/test_login_integration.py (1)
website/tests/test_login_functionality.py (5)
setUp(11-29)test_empty_credentials(82-93)test_case_insensitive_email_login(181-189)test_csrf_protection(247-261)test_remember_me_functionality(293-306)
website/tests/test_login_functionality.py (3)
website/tests/test_login_integration.py (4)
setUp(24-70)test_empty_credentials(152-163)test_csrf_protection(184-200)test_remember_me_functionality(329-341)website/static/js/repo_detail.js (1)
response(103-110)website/models.py (1)
is_active(3389-3394)
🪛 LanguageTool
website/templates/account/email/email_confirmation_message.txt
[style] ~13-~13: Consider using a different verb to strengthen your wording.
Context: ...If you didn't create an account, please ignore this email. Welcome to {{ site_name }}...
(IGNORE_DISREGARD)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (11)
website/templates/account/email/email_confirmation_subject.txt (1)
1-4: LGTM!The email subject template follows proper Django i18n patterns with
blocktransfor translation support. Usingautoescape offis appropriate for plain text email subjects.website/templates/account/verification_sent.html (1)
40-40: LGTM!The updated message correctly reflects the email verification context by replacing "finalize the signup process" with "complete the email verification process", addressing the issue requirement to remove misleading "signup" references.
website/templates/account/email/email_confirmation_message.txt (1)
5-18: LGTM!The email confirmation message is well-structured with proper i18n support. The content is clear and professional, including all essential elements: greeting, purpose explanation, activation link, fallback instruction, and sign-off. The static analysis hint about "ignore" is a false positive—this is standard and appropriate phrasing for verification emails.
test_login_standalone.py (1)
139-189: Security tests validate pattern detection, not actual Django protection.These tests verify that string pattern matching works, but they don't test the actual Django/allauth protections. The SQL injection and XSS protection in Django comes from parameterized queries and template auto-escaping, not string pattern matching. Consider adding a note in the README clarifying that these are conceptual tests demonstrating expected behaviors, not tests of the actual security mechanisms.
website/templates/account/login.html (2)
71-77: Good addition of non-field error display.The error display block properly shows form-level validation errors with appropriate styling. This addresses the PR objective of displaying login errors prominently.
87-90: Good implementation of field-level error highlighting and messages.The conditional
border-red-500class and error message display for both login and password fields provide clear visual feedback for validation errors.Also applies to: 105-105, 120-122
blt/settings.py (1)
367-370: Custom adapter and form modules are properly implemented.The settings configuration correctly references existing classes:
website.account_adapters.CustomAccountAdapter(line 11 of website/account_adapters.py),website.forms.SignupFormWithCaptcha(line 171 of website/forms.py), andwebsite.forms.CustomLoginForm(line 24 of website/forms.py). All custom modules are in place and properly integrated.website/account_adapters.py (2)
16-26: LGTM!The
add_messageoverride correctly normalizes login failure and email verification messages for improved UX, and properly delegates to the parent implementation.
34-45: No issues found. Thesend_confirmation_mailmethod is syntactically complete—the closing parenthesis formessages.success()is present on line 45. The code requires no fixes.Likely an incorrect or invalid review comment.
website/tests/test_login_functionality.py (1)
11-29: LGTM!The test setup correctly initializes the test client, creates a test user, and sets up a verified email address for allauth integration.
website/tests/test_login_integration.py (1)
24-70: LGTM!Comprehensive test data setup with three distinct user states (active, inactive, unverified) that cover the main authentication scenarios.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
- Format Python files with black - Sort imports with isort - Ensure compliance with pre-commit hooks
github-actions
bot
added
changes-requested
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
website/templates/account/login.html
Outdated
| name="{{ redirect_field_name }}" | ||
| value="{{ redirect_field_value }}" /> | ||
| {% endif %} | ||
| <form class="flex flex-col space-y-4 login" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/tests/test_login_integration.py (1)
263-274: Incomplete remember-me test - no assertion for session behavior.The test only verifies the login succeeds with
remember: 'on'but doesn't assert any difference in session behavior. Consider adding an assertion to verify the session does not expire at browser close when remember-me is checked.🔎 Suggested improvement
def test_remember_me_functionality(self): """Test remember me checkbox if available""" - response = self.client.post( - self.login_url, {"login": "activeuser", "password": "securepass123", "remember": "on"} - ) + response = self.client.post(self.login_url, { + 'login': 'activeuser', + 'password': 'securepass123', + 'remember': 'on' + }) # Should redirect after successful login self.assertEqual(response.status_code, 302) - # Session expiry should be configured appropriately - # This test depends on the specific implementation + # When remember me is checked, session should NOT expire at browser close + self.assertFalse(self.client.session.get_expire_at_browser_close())website/tests/test_login_functionality.py (1)
225-234: Incorrect assertion for remember-me functionality.The assertion
self.assertTrue(self.client.session.get_expire_at_browser_close() is not None)will always pass becauseget_expire_at_browser_close()returns a boolean (TrueorFalse), neverNone. When "remember me" is checked, the session should persist beyond browser close, meaningget_expire_at_browser_close()should returnFalse.🔎 Proposed fix
def test_remember_me_functionality(self): """Test remember me checkbox functionality""" - response = self.client.post(self.login_url, {"login": "testuser", "password": "testpass123", "remember": "on"}) + response = self.client.post(self.login_url, { + 'login': 'testuser', + 'password': 'testpass123', + 'remember': 'on' + }) # Should redirect after successful login self.assertEqual(response.status_code, 302) - # Session should be configured for longer duration - # This is implementation-specific and might need adjustment - self.assertTrue(self.client.session.get_expire_at_browser_close() is not None) + # When remember me is checked, session should NOT expire at browser close + self.assertFalse(self.client.session.get_expire_at_browser_close())
🧹 Nitpick comments (1)
PR_DESCRIPTION.md (1)
47-54: Add language specifier to code block.The fenced code block showing file changes should specify a language (e.g.,
diffortext) for better rendering and accessibility.🔎 Suggested fix
### 📊 Files Changed -``` +```diff blt/settings.py | 7 +++-- website/forms.py | 28 +++++++++++++++++-
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
PR_DESCRIPTION.mdwebsite/account_adapters.pywebsite/forms.pywebsite/tests/test_login_functionality.pywebsite/tests/test_login_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- website/forms.py
🧰 Additional context used
🧬 Code graph analysis (2)
website/tests/test_login_integration.py (3)
website/tests/test_login_functionality.py (5)
setUp(11-20)test_empty_credentials(61-69)test_case_insensitive_email_login(130-135)test_csrf_protection(188-199)test_remember_me_functionality(225-234)website/models.py (1)
is_active(3389-3394)website/static/js/repo_detail.js (1)
response(103-110)
website/tests/test_login_functionality.py (3)
website/tests/test_login_integration.py (1)
setUp(25-51)website/static/js/repo_detail.js (1)
response(103-110)website/models.py (1)
is_active(3389-3394)
🪛 LanguageTool
PR_DESCRIPTION.md
[uncategorized] ~37-~37: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...rotections**: SQL injection, XSS, CSRF, rate limiting - UI/UX: Error messages, redirects,...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~44-~44: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ndling without information disclosure - Rate limiting simulation tests ### 📊 Files Changed ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
PR_DESCRIPTION.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
| def add_message(self, request, level, message_tag, message, **kwargs): | ||
| """ | ||
| Override to provide cleaner error messages for login failures. | ||
| """ | ||
| # Clean up generic error messages for better UX | ||
| if message_tag == "account_login_failed": | ||
| message = "Invalid username/email or password. Please check your credentials and try again." | ||
| elif message_tag == "account_email_verification_sent": | ||
| message = "We've sent a verification email to your address. Please check your inbox and click the verification link to activate your account." | ||
|
|
||
| return super().add_message(request, level, message_tag, message, **kwargs) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
django-allauth 65.13.1 DefaultAccountAdapter add_message method signature
💡 Result:
Signature:
add_message(request, level, message_template=None, message_context=None, extra_tags='', message=None). [1][2]
It's a wrapper around django.contrib.messages.add_message that reads message text from a template. [1]
🏁 Script executed:
# First, let's check the actual file in the repository
cat -n website/account_adapters.pyRepository: OWASP-BLT/BLT
Length of output: 2103
Critical: Method signature doesn't match parent class.
The add_message method signature doesn't match the parent DefaultAccountAdapter.add_message in django-allauth 65.13.1. The parent method signature is:
def add_message(self, request, level, message_template=None, message_context=None, extra_tags='', message=None)Your current signature uses different parameter names (message_tag instead of message_template). When django-allauth internally calls this method, it will pass arguments matching the parent signature, causing the custom method to fail.
Update the method signature and adjust the logic to work with message_template:
def add_message(self, request, level, message_template=None, message_context=None, extra_tags='', message=None):
"""
Override to provide cleaner error messages for login failures.
"""
# Clean up generic error messages for better UX
if message_template == "account/messages/logged_in.txt":
message = "Invalid username/email or password. Please check your credentials and try again."
elif message_template == "account/messages/email_confirmation_sent.txt":
message = "We've sent a verification email to your address. Please check your inbox and click the verification link to activate your account."
return super().add_message(request, level, message_template, message_context, extra_tags, message)Note: Verify the exact message template names used by django-allauth for these flows—they may differ from those shown above.
- Remove standalone test file with linting violations - Fix HTML template formatting with djLint - Ensure all code passes pre-commit hooks
github-actions
bot
added
files-changed: 11
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
website/tests/test_login_integration.py (1)
261-271: Incomplete remember-me test lacks session behavior assertion.The test only verifies that login succeeds with
remember: 'on'but doesn't assert any difference in session behavior compared to a login without "remember me". Consider adding an assertion to verify the session expiry configuration.🔎 Suggested improvement
def test_remember_me_functionality(self): """Test remember me checkbox if available""" response = self.client.post( self.login_url, {"login": "activeuser", "password": "securepass123", "remember": "on"} ) # Should redirect after successful login self.assertEqual(response.status_code, 302) - # Session expiry should be configured appropriately - # This test depends on the specific implementation + # When remember me is checked, session should NOT expire at browser close + self.assertFalse(self.client.session.get_expire_at_browser_close())website/templates/account/login.html (1)
60-68: Critical: Remove duplicate nested<form>tag - this creates invalid HTML.Lines 60-68 introduce a second
<form>tag nested inside the form that starts at line 51. Nested forms are invalid HTML and will cause unpredictable browser behavior—most browsers will ignore the inner form tag, leading to form submission failures.Remove the duplicate form opening tag (lines 60-68) while keeping the form content that follows.
🔎 Proposed fix - remove duplicate form tag
{% if redirect_field_value %} <input type="hidden" name="{{ redirect_field_name }}" value="{{ redirect_field_value }}" /> {% endif %} - <form class="flex flex-col space-y-4 login" - method="post" - action="{% url 'account_login' %}?next={{ request.path }}"> - {% csrf_token %} - {% if redirect_field_value %} - <input type="hidden" - name="{{ redirect_field_name }}" - value="{{ redirect_field_value }}" /> - {% endif %} <!-- Display form errors -->
🧹 Nitpick comments (4)
website/templates/account/login.html (2)
75-79: Label correctly updated to "Username or Email".This addresses the PR objective to allow sign-in using either username or email. However, this label text should be wrapped with
{% trans %}for internationalization consistency with other labels in the template.🔎 Proposed i18n fix
<label for="id_login" class="text-sm font-semibold text-gray-500 dark:text-gray-500"> - Username or Email + {% trans "Username or Email" %} </label>
126-129: "Remember me" label should use translation tag for consistency.Other text in the template uses
{% trans %}for internationalization. The "Remember me" label should follow the same pattern.🔎 Proposed i18n fix
<label for="id_remember" class="mb-0 text-sm font-semibold text-gray-500 dark:text-gray-500 select-none"> - Remember me + {% trans "Remember me" %} </label>website/tests/test_login_integration.py (2)
245-259: Rate limiting test may not effectively verify the behavior.The test uses
@override_settings(ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3)but then makes 5 failed attempts and expects status 200 for all of them. The final assertionself.assertIn(response.status_code, [200, 302])is overly permissive and doesn't verify that rate limiting actually kicked in.If rate limiting is implemented, consider asserting a specific behavior (e.g., error message about too many attempts, or a 429 status). If rate limiting isn't implemented yet, this test provides limited value.
🔎 Suggested improvement
@override_settings(ACCOUNT_LOGIN_ATTEMPTS_LIMIT=3) def test_rate_limiting_simulation(self): - """Test rate limiting behavior (if implemented)""" + """Test rate limiting behavior""" # Make multiple failed login attempts - for i in range(5): + for i in range(3): response = self.client.post(self.login_url, {"login": "activeuser", "password": "wrongpassword"}) - - # Should stay on login page self.assertEqual(response.status_code, 200) - # Valid login should still work (basic test) + # After exceeding limit, should show rate limit message or block response = self.client.post(self.login_url, {"login": "activeuser", "password": "securepass123"}) - - # Should either work or show rate limit message - self.assertIn(response.status_code, [200, 302]) + # Verify rate limiting is enforced (adjust based on actual implementation) + if response.status_code == 200: + self.assertContains(response, "Too many login attempts")
320-329: tearDown cleanup is unnecessary with Django's TestCase.Django's
TestCasewraps each test in a transaction that is rolled back after the test, automatically cleaning up database objects. Manual deletion intearDownis redundant and adds unnecessary overhead.🔎 Suggested simplification
def tearDown(self): - """Clean up after tests""" - # Clear any remaining sessions - Session.objects.all().delete() - - # Clear users - User.objects.all().delete() - - # Clear email addresses - EmailAddress.objects.all().delete() + """Clean up after tests""" + # Django's TestCase automatically rolls back transactions, + # so manual cleanup is not required + passOr simply remove the
tearDownmethod entirely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
blt/settings.pywebsite/templates/account/login.htmlwebsite/tests/test_login_integration.py
🧰 Additional context used
🧬 Code graph analysis (1)
website/tests/test_login_integration.py (1)
website/tests/test_login_functionality.py (5)
setUp(11-20)test_empty_credentials(61-69)test_case_insensitive_email_login(130-135)test_csrf_protection(188-199)test_remember_me_functionality(225-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (8)
website/templates/account/login.html (2)
69-86: Good implementation of error display with proper visual feedback.The non-field errors are displayed in a prominent red alert box, and per-field errors show beneath inputs with red text. The conditional red border styling on inputs with errors (
{% if form.login.errors %} border-red-500{% endif %}) provides clear visual feedback. This addresses the PR objective of improving error visibility.
174-178: Verify closing tag structure after fixing the duplicate form.Once the duplicate
<form>tag is removed (lines 60-68), ensure the closing tags are balanced. Currently there's</form>at line 174 and</div>at lines 175-176, which should close the form and container divs correctly. The structure should be:
</form>closes the form (line 51)</div>closes.p-5.bg-white(line 49)</div>closes.flex.flex-col.overflow-hidden(line 25)</div>closes.flex.items-center.min-h-screen(line 24)website/tests/test_login_integration.py (4)
23-49: Well-structured test setup with appropriate user scenarios.The setUp creates users for key test scenarios: active user with verified email, inactive user, and unverified user. The EmailAddress records are correctly created to work with allauth's email verification system.
229-243: Logout test is correct but could optionally cover GET logout.The test correctly verifies logout via POST. Given
ACCOUNT_LOGOUT_ON_GET = Truein settings, you might also want to verify GET-based logout works. However, the current test is sufficient for basic logout verification.
154-171: SQL injection tests are good for documentation but Django ORM provides protection.These tests verify that SQL injection payloads don't compromise the database. While Django's ORM inherently protects against SQL injection through parameterized queries, having these tests serves as regression protection and documents security expectations.
173-189: XSS protection test approach is sound.The test verifies that malicious payloads are escaped in the response using
assertNotContains(response, payload, html=False). This correctly ensures raw script tags and event handlers don't appear in the rendered output.blt/settings.py (2)
550-550: Custom account adapter path updated correctly.The adapter is now pointed to
website.account_adapters.CustomAccountAdapterto support the enhanced login messaging.
367-367: ACCOUNT_FORMS configuration is correctly mapped to existing form classes.The custom forms are properly defined in
website/forms.py:
CustomLoginFormat line 24SignupFormWithCaptchaat line 175The django-allauth integration follows the standard pattern for form overrides.
github-actions
bot
removed
changes-requested
github-actions
bot
added
pre-commit: failed
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
angeliaaju
force-pushed
the
issue-4056-error-messages-verify-email-ui
branch
from
95dc91c to
2ad6211
Compare
github-actions
bot
added
files-changed: 11
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
github-actions
bot
added
last-active: 0d
|
💬 Reminder: Unresolved Conversations Hi @angeliaaju! This pull request has 1 unresolved conversation that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
github-actions
bot
added
last-active: 1d
|
💬 Reminder: Unresolved Conversations Hi @angeliaaju! This pull request has 1 unresolved conversation that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
1 participant
Fix Login Error Messages and Email Verification UI
Fixes #4056
Summary
This PR addresses the login error message display issues and improves the email verification UI as requested in issue #4056.
🔧 Changes Made
Login Error Messages
CustomLoginFormwith clear error handling inwebsite/forms.py📧Email Verification UI
Configuration
blt/settings.pyTesting
Added comprehensive test suite covering:
Test Files
test_login_standalone.py- Standalone tests (no Django deps)website/tests/test_login_functionality.py- Django unit testswebsite/tests/test_login_integration.py- Full integration testsLOGIN_TESTS_README.md- Complete test documentationTest Coverage
Files Changed
Before & After
Before:
After:
Testing Instructions
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.