-
Notifications
You must be signed in to change notification settings - Fork 66
feat: throw custom authentication exception #105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request updates the PHP version requirement from >=8.0.0 to >=8.1 in the project manifest. It introduces a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
| }, | ||
| "require": { | ||
| "php": ">=8.0.0", | ||
| "php": ">=8.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use enums in the repo so updated min php requirement
| private bool $smtpAutoTLS = false, | ||
| private string $xMailer = '', | ||
| private int $timeout = 30 | ||
| private int $connectTimeout = 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed naming to make it clearer what the timeout meant and difference between it ad timelimit (by default both are 5min in phpmailer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @src/Utopia/Messaging/Adapter/Email/SMTP.php:
- Around line 120-125: The authentication check is too strict: instead of
comparing $errorInfo === 'authenticate' inside the conditional that checks
!$sent and !empty($mail->ErrorInfo), change the equality match to a substring
check (e.g., use str_contains on the lowercased $errorInfo for "authenticate" or
"auth" or use strpos !== false) so PHPMailer messages like "SMTP Error: Could
not authenticate" correctly trigger throwing the AuthenticationException; update
the conditional that inspects $mail->ErrorInfo/$errorInfo to use that substring
check and keep throwing AuthenticationException as before.
In @tests/Messaging/Adapter/Email/SMTPTest.php:
- Around line 119-146: The authentication error check in the SMTP adapter (in
the SMTP class where $errorInfo is derived from $mail->ErrorInfo) uses a strict
equality against 'authenticate' and thus never matches full messages like "SMTP
Error: Could not authenticate"; update the check to look for the substring
instead (e.g., use str_contains on a lowercased $errorInfo or strpos !== false)
so authentication-related messages trigger throwing AuthenticationException in
the send flow.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
composer.jsonsrc/Utopia/Messaging/Adapter/Email/SMTP.phpsrc/Utopia/Messaging/Exception/AuthenticationException.phptests/Messaging/Adapter/Email/SMTPTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Utopia/Messaging/Adapter/Email/SMTP.php (1)
src/Utopia/Messaging/Exception/AuthenticationException.php (1)
AuthenticationException(5-7)
tests/Messaging/Adapter/Email/SMTPTest.php (2)
src/Utopia/Messaging/Exception/AuthenticationException.php (1)
AuthenticationException(5-7)src/Utopia/Messaging/Adapter/Email/SMTP.php (1)
SMTP(11-158)
🪛 GitHub Actions: Tests
tests/Messaging/Adapter/Email/SMTPTest.php
[error] 1-1: PHPUnit test failed: testSendEmailWithAuthenticationFailure did not throw Utopia\Messaging\Exception\AuthenticationException as expected.
🔇 Additional comments (6)
src/Utopia/Messaging/Exception/AuthenticationException.php (1)
1-7: LGTM!The exception class is correctly defined and follows standard PHP exception patterns. It appropriately extends
\Exceptionand provides a specific exception type for authentication failures.tests/Messaging/Adapter/Email/SMTPTest.php (1)
6-6: LGTM!The import is correctly added to support the new authentication exception test.
src/Utopia/Messaging/Adapter/Email/SMTP.php (3)
7-7: LGTM!The import is correctly added to support throwing authentication exceptions.
23-25: No breaking change evident in the current codebase.The constructor already includes separate
connectTimeoutandtimelimitparameters with sensible defaults (10 and 30 seconds respectively). Git history shows no evidence of a previous singletimeoutparameter, and all existing test instantiations use named parameters that are compatible with the current signature. No breaking change has occurred.Likely an incorrect or invalid review comment.
68-69: Timeout configuration verified and correct.The implementation is correct for PHPMailer 6.9.1:
$mail->Timeoutproperly sets the SMTP connection timeout on the PHPMailer object$mail->getSMTPInstance()->Timelimitcorrectly sets the per-command response timeout on the SMTP instance- Both properties exist and function as documented in PHPMailer 6.9.1
composer.json (1)
25-25: PHP 8.1 requirement is justified by the use of backed enums.The codebase uses backed enums (specifically
enum Priority: intinsrc/Utopia/Messaging/Priority.php), which were introduced in PHP 8.1 and are not available in PHP 8.0. The version bump is necessary and cannot be reverted without removing this language feature.Likely an incorrect or invalid review comment.
| if (!$sent && !empty($mail->ErrorInfo)) { | ||
| $errorInfo = \strtolower($mail->ErrorInfo); | ||
| if ($errorInfo === 'authenticate') { | ||
| throw new AuthenticationException('SMTP Error: Could not authenticate'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical bug: Authentication error detection logic is too strict.
The comparison $errorInfo === 'authenticate' is causing the test to fail because PHPMailer's ErrorInfo typically contains more descriptive messages like "SMTP Error: Could not authenticate." or "Authentication failed" rather than just the word "authenticate".
Use a substring check instead of exact equality:
🔎 Proposed fix using str_contains
- if (!$sent && !empty($mail->ErrorInfo)) {
- $errorInfo = \strtolower($mail->ErrorInfo);
- if ($errorInfo === 'authenticate') {
- throw new AuthenticationException('SMTP Error: Could not authenticate');
- }
- }
+ if (!$sent && !empty($mail->ErrorInfo)) {
+ $errorInfo = \strtolower($mail->ErrorInfo);
+ if (\str_contains($errorInfo, 'authenticate') || \str_contains($errorInfo, 'authentication')) {
+ throw new AuthenticationException('SMTP Error: Could not authenticate');
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!$sent && !empty($mail->ErrorInfo)) { | |
| $errorInfo = \strtolower($mail->ErrorInfo); | |
| if ($errorInfo === 'authenticate') { | |
| throw new AuthenticationException('SMTP Error: Could not authenticate'); | |
| } | |
| } | |
| if (!$sent && !empty($mail->ErrorInfo)) { | |
| $errorInfo = \strtolower($mail->ErrorInfo); | |
| if (\str_contains($errorInfo, 'authenticate') || \str_contains($errorInfo, 'authentication')) { | |
| throw new AuthenticationException('SMTP Error: Could not authenticate'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/Utopia/Messaging/Adapter/Email/SMTP.php around lines 120-125, The
authentication check is too strict: instead of comparing $errorInfo ===
'authenticate' inside the conditional that checks !$sent and
!empty($mail->ErrorInfo), change the equality match to a substring check (e.g.,
use str_contains on the lowercased $errorInfo for "authenticate" or "auth" or
use strpos !== false) so PHPMailer messages like "SMTP Error: Could not
authenticate" correctly trigger throwing the AuthenticationException; update the
conditional that inspects $mail->ErrorInfo/$errorInfo to use that substring
check and keep throwing AuthenticationException as before.
| public function testSendEmailWithAuthenticationFailure(): void | ||
| { | ||
| $this->expectException(AuthenticationException::class); | ||
| $this->expectExceptionMessage('SMTP Error: Could not authenticate'); | ||
|
|
||
| $sender = new SMTP( | ||
| host: 'maildev', | ||
| port: 1025, | ||
| username: 'invalid_user', | ||
| password: 'invalid_password', | ||
| ); | ||
|
|
||
| $to = 'tester@localhost.test'; | ||
| $subject = 'Test Subject'; | ||
| $content = 'Test Content'; | ||
| $fromName = 'Test Sender'; | ||
| $fromEmail = 'sender@localhost.test'; | ||
|
|
||
| $message = new Email( | ||
| to: [$to], | ||
| subject: $subject, | ||
| content: $content, | ||
| fromName: $fromName, | ||
| fromEmail: $fromEmail, | ||
| ); | ||
|
|
||
| $sender->send($message); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the SMTP adapter file
fd "SMTP.php" --type f | head -20Repository: utopia-php/messaging
Length of output: 107
🏁 Script executed:
# Let's look at the actual SMTP adapter implementation
cat -n "src/Utopia/Messaging/Adapter/Email/SMTP.php" | head -150Repository: utopia-php/messaging
Length of output: 6742
🏁 Script executed:
# Also check the test file to understand the full context
cat -n "tests/Messaging/Adapter/Email/SMTPTest.php" | sed -n '100,160p'Repository: utopia-php/messaging
Length of output: 1772
🏁 Script executed:
# Search for PHPMailer ErrorInfo usage and patterns
rg -n "ErrorInfo|authenticate" --type=php src/Utopia/Messaging/Adapter/Email/Repository: utopia-php/messaging
Length of output: 1535
🏁 Script executed:
# Check if there are any other references to error handling in tests
rg -n "Exception|Error" --type=php tests/Messaging/Adapter/Email/SMTPTest.php | head -30Repository: utopia-php/messaging
Length of output: 270
🏁 Script executed:
# Look for any documentation about PHPMailer in comments or docs
rg -n "PHPMailer|authentication" --type=php --type=md . | grep -i "error\|auth" | head -20Repository: utopia-php/messaging
Length of output: 46
🏁 Script executed:
# Search for any comments or documentation about PHPMailer error messages
rg -n "Could not authenticate|SMTP Error" --type=php .Repository: utopia-php/messaging
Length of output: 328
🏁 Script executed:
# Check if there's a composer.json to see what version of PHPMailer is used
cat composer.json | grep -A5 -B5 "phpmailer"Repository: utopia-php/messaging
Length of output: 319
🏁 Script executed:
# Look for any other tests or examples that might show what error PHPMailer actually returns
find . -type f -name "*.php" -exec grep -l "PHPMailer\|ErrorInfo" {} \; 2>/dev/null | head -10Repository: utopia-php/messaging
Length of output: 155
🏁 Script executed:
# Check if vendor directory exists and look for PHPMailer error messages
find vendor -path "*/phpmailer/phpmailer/*" -name "*.php" -type f 2>/dev/null | head -5Repository: utopia-php/messaging
Length of output: 46
🏁 Script executed:
# Search in vendor for authentication error handling
rg -n "Could not authenticate|SMTP.*Error" vendor/phpmailer/ --type=php 2>/dev/null | head -10Repository: utopia-php/messaging
Length of output: 46
🏁 Script executed:
# Let's check the SMTP class in PHPMailer for error messages
rg -n "authenticate" vendor/phpmailer/phpmailer/src/SMTP.php --type=php -A2 -B2 2>/dev/null | head -30Repository: utopia-php/messaging
Length of output: 46
🏁 Script executed:
# Let's look for any documentation or comments in the SMTP file about expected error messages
cat -n "src/Utopia/Messaging/Adapter/Email/SMTP.php" | grep -A10 -B10 "ErrorInfo\|authenticate"Repository: utopia-php/messaging
Length of output: 1946
🏁 Script executed:
# Check if there are any GitHub issues, documentation files, or comments about this
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) -exec grep -l "PHPMailer\|SMTP" {} \; 2>/dev/nullRepository: utopia-php/messaging
Length of output: 46
🏁 Script executed:
# Let's check the git history to see if there were any recent changes to this error handling
git log --oneline -20 -- src/Utopia/Messaging/Adapter/Email/SMTP.php 2>/dev/null || echo "Git history not available"Repository: utopia-php/messaging
Length of output: 115
🏁 Script executed:
# Let's check that git commit to see what was the original intention
git show c5d96af --stat 2>/dev/null || echo "Git show not available"Repository: utopia-php/messaging
Length of output: 5228
🏁 Script executed:
# Check the entire git log with more context
git log -p --all -- src/Utopia/Messaging/Adapter/Email/SMTP.php 2>/dev/null | head -100 || echo "Git log not available"Repository: utopia-php/messaging
Length of output: 46
🏁 Script executed:
# Let's search for any test setup or mocking that might show what error PHPMailer returns
cat -n tests/Messaging/Adapter/Email/SMTPTest.php | head -50Repository: utopia-php/messaging
Length of output: 1793
🏁 Script executed:
# Let's check the entire test file to understand test setup and dependencies
wc -l tests/Messaging/Adapter/Email/SMTPTest.phpRepository: utopia-php/messaging
Length of output: 110
🏁 Script executed:
# Check the Base test class to understand test setup
cat -n tests/Messaging/Adapter/Base.phpRepository: utopia-php/messaging
Length of output: 1579
🌐 Web query:
PHPMailer 6.9.1 authentication failure error message SMTP
💡 Result:
Common causes and fixes for "SMTP authentication" failures with PHPMailer 6.9.1
-
Wrong credentials or quoting issues
- Ensure username/password are correct and properly quoted/escaped in PHP (use single quotes or escape $). [1][2]
-
Provider-side blocks (Google/Gmail 534 / "Please log in via your web browser")
- Google often blocks sign-ins from apps it considers less secure. Fixes: enable app password (if 2FA), use DisplayUnlockCaptcha, reset password, or migrate to OAuth2/XOAUTH2. The SMTP server response (e.g. 534‑5.7.14) will show this. [3][4]
-
TLS/port/mode mismatch
- Use the correct combination: SMTPS (ssl) on port 465, STARTTLS (tls) on port 587; set SMTPAutoTLS appropriately. PHPMailer 6.9.1 changed opportunistic STARTTLS behavior for localhost (you may still need SMTPAutoTLS = false when connecting to literal IPs). [1]
-
Need to move to OAuth2/XOAUTH2 for some providers
- Many providers (Google, Microsoft) now require OAuth2 for long-term, secure authentication; PHPMailer links to XOAUTH2 support in v6.9.1. Consider implementing XOAUTH2 instead of plain SMTP auth. [1]
-
Debugging steps (recommended)
- Turn on SMTP debugging to see server replies: $mail->SMTPDebug = SMTP::DEBUG_SERVER; and inspect the exact server error (it usually indicates the real cause).
- Verify network/port connectivity (telnet/openssl s_client) and that your hosting provider allows outbound SMTP. [2][3]
Quick checklist to resolve:
- Verify credentials and quoting.
- Confirm host, port, and encryption match provider docs.
- If using Gmail: enable app password/DisplayUnlockCaptcha or use XOAUTH2.
- Check PHPMailer SMTP debug output for the server's error text and act on it.
- If connecting to localhost/IP, try $mail->SMTPAutoTLS = false (if opportunistic STARTTLS is failing). [1][3][4]
Sources:
[1] PHPMailer 6.9.1 release notes. [2] Stack Overflow — "SMTP Error: Could not authenticate" common fixes. [3] PHPMailer GitHub issue showing Gmail 534 response. [4] Stack Overflow — Gmail 534 / DisplayUnlockCaptcha solutions.
Fix error detection logic for authentication failures in SMTP adapter.
The issue is at line 122 in src/Utopia/Messaging/Adapter/Email/SMTP.php. The condition if ($errorInfo === 'authenticate') performs a strict equality check, but $mail->ErrorInfo for authentication failures contains a full error message like "SMTP Error: Could not authenticate". When converted to lowercase, this becomes "smtp error: could not authenticate", which never equals "authenticate", so the condition never matches.
The AuthenticationException is never thrown, causing the test to fail. Replace the strict comparison with str_contains($errorInfo, 'authenticate') to detect authentication-related errors regardless of message format.
🤖 Prompt for AI Agents
In @tests/Messaging/Adapter/Email/SMTPTest.php around lines 119-146, The
authentication error check in the SMTP adapter (in the SMTP class where
$errorInfo is derived from $mail->ErrorInfo) uses a strict equality against
'authenticate' and thus never matches full messages like "SMTP Error: Could not
authenticate"; update the check to look for the substring instead (e.g., use
str_contains on a lowercased $errorInfo or strpos !== false) so
authentication-related messages trigger throwing AuthenticationException in the
send flow.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.