Fix smtp auth check #10939

stnguyen90 · 2025-12-12T05:00:00Z

What does this PR do?

Set $mail->SMTPAuth to true to ensure the update SMTP endpoint checks for successful authentication before saving.

This PR also updates maildev to enable authentication so that the tests can actually test for success and failure.

Fixes #9067

Test Plan

Test cases added to test invalid SMTP credentials.

Related PRs and Issues

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

Fix: Implement mandatory authentication check for Custom SMTP configuration

coderabbitai · 2025-12-12T05:00:06Z

📝 Walkthrough

Walkthrough

Defaults for SMTP credentials were added to environment configuration. The projects API now sets PHPMailer->SMTPAuth when both username and password are present and rethrows SMTP connection errors with the original message. docker-compose was updated to pass SMTP credentials into the maildev service. End-to-end tests and test fixtures were updated to read SMTP host/port/username/password from environment variables, to include an invalid-credentials failure assertion, and to adjust several expectations and counts accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

app/controllers/api/projects.php — verify SMTPAuth conditional, connection/test flow, and changed exception message semantics.
docker-compose.yml and .env — confirm added Maildev env mappings and default credential values.
tests/e2e/Scopes/ProjectCustom.php — ensure environment-driven SMTP defaults are correct and safe for CI.
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php — validate updated assertions, counts, SMTP success/failure cases, and reordered assertions.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix smtp auth check' directly addresses the main change: enabling SMTPAuth validation during SMTP connection testing.
Description check	✅ Passed	The description explains the purpose of the changes (enabling SMTPAuth, updating maildev config) and references the linked issue #9067.
Linked Issues check	✅ Passed	The PR successfully addresses issue #9067 by setting SMTPAuth to true during SMTP connection testing and updating maildev configuration to validate credentials.
Out of Scope Changes check	✅ Passed	All changes are in scope: SMTP auth enforcement, maildev credential setup, environment variable configuration, and test updates to validate the fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-smtp-auth-check

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-12-12T05:02:35Z

Security Scan Results for PR

Docker Image Scan Results

Package	Version	Vulnerability	Severity
libpng	1.6.51-r0	CVE-2025-66293	HIGH
libpng-dev	1.6.51-r0	CVE-2025-66293	HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

github-actions · 2025-12-12T05:36:05Z

✨ Benchmark results

Requests per second: 1,186
Requests with 200 status code: 213,438
P99 latency: 0.163573253

⚡ Benchmark Comparison

Metric	This PR	Latest version
RPS	1,186	1,257
200	213,438	226,341
P99	0.163573253	0.162536221

stnguyen90 · 2025-12-12T05:51:32Z

@copilot the latest CI tests (https://github.com/appwrite/appwrite/actions/runs/20157261474) are failing. Fix the tests.

Copilot · 2025-12-12T05:51:40Z

@stnguyen90 I've opened a new pull request, #10940, to work on those changes. Once the pull request is ready, I'll request review from you.

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)

260-277: Brittle assertions: hard-coded project totals/order will flake if test order/data changes.
These expectations (e.g., total === 4, count === 5, and specific name ordering at offsets) implicitly depend on global state created by other tests and/or list ordering guarantees that may not hold across environments. Prefer making this test self-contained (create the projects it expects) or asserting relative changes (baseline + N), and avoid relying on ordering unless you explicitly order by a stable field.

Suggested direction (illustrative): record baseline total before creating any projects in this test, then assert baseline + createdCount, and use Query::orderAsc('$id') (or another stable field) when asserting order.

Also applies to: 351-353, 379-392, 405-407

🧹 Nitpick comments (2)

tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (2)

621-687: SMTP update test: good coverage; make failure assertion less message-fragile + verify config unchanged after failure.

assertStringContainsStringIgnoringCase('Could not authenticate', ...) may vary by PHPMailer/SMTP server; consider asserting only the error type (and maybe that message contains authenticate) to reduce flakes.

After the invalid-credentials attempt returns 400, consider fetching /projects/{id} again and asserting SMTP settings remain the previously-set valid config (guards against partial writes on failure).

621-625: Env-driven SMTP params: good; reduce duplication via a small helper/fixture.
The same _APP_SMTP_* env reads are repeated in multiple tests; extracting to a private helper (or reusing an existing fixture) will keep future SMTP changes localized.

Also applies to: 696-700, 735-739, 772-776, 788-792

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 278679a and d451738.

📒 Files selected for processing (5)

.env (1 hunks)
app/controllers/api/projects.php (4 hunks)
docker-compose.yml (1 hunks)
tests/e2e/Scopes/ProjectCustom.php (1 hunks)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (18 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

app/controllers/api/projects.php (1)

src/Appwrite/Extend/Exception.php (1)

Exception (7-472)

tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (4)

tests/e2e/Scopes/ProjectCustom.php (1)

getProject (21-184)

src/Appwrite/Event/Event.php (1)

getProject (153-156)

tests/e2e/Scopes/Scope.php (1)

getProject (150-150)

src/Appwrite/Extend/Exception.php (1)

Exception (7-472)

🪛 dotenv-linter (4.0.0)

.env

[warning] 72-72: [UnorderedKey] The _APP_SMTP_USERNAME key should go before the _APP_STORAGE_ANTIVIRUS key

(UnorderedKey)

[warning] 73-73: [UnorderedKey] The _APP_SMTP_PASSWORD key should go before the _APP_SMTP_PORT key

(UnorderedKey)

🔇 Additional comments (12)

docker-compose.yml (1)

1113-1115: LGTM - Maildev authentication configuration aligns with SMTP auth fix.

Configuring MAILDEV_INCOMING_USER and MAILDEV_INCOMING_PASS allows the development mail server to enforce authentication, enabling proper testing of the SMTP credential validation fix.

.env (1)

72-73: LGTM - Default SMTP credentials for development environment.

Setting default values for _APP_SMTP_USERNAME and _APP_SMTP_PASSWORD coordinates with the maildev container configuration and enables SMTP authentication testing. The static analysis warnings about key ordering can be safely ignored as alphabetical ordering is not a functional requirement for .env files.

tests/e2e/Scopes/ProjectCustom.php (1)

164-166: LGTM - Environment-based SMTP configuration for tests.

Using System::getEnv with fallback defaults provides flexibility while maintaining consistency with the .env defaults. The intval() wrapper for port is appropriate since environment variables are strings.

app/controllers/api/projects.php (4)

2082-2082: Core fix correctly enables SMTP authentication validation.

Setting SMTPAuth based on the presence of both username and password ensures credentials are actually validated during SmtpConnect(). This addresses the root cause identified in issue #9067 where SMTP settings were being saved without validating authentication credentials.

2098-2098: Improved error message propagation.

Passing the original exception message directly provides more specific and actionable error details (e.g., "SMTP Error: Could not authenticate" from PHPMailer) rather than a generic wrapper message.

297-297: Minor formatting adjustment - no functional change.

2669-2669: Minor formatting adjustment - no functional change.

tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (5)

25-28: Test grouping change looks fine; just ensure CI uses these groups consistently.

812-813: Updated default template subject expectation is consistent with the new baseline.

915-916: Auth duration assertion update looks correct given the project name in this flow.

3135-3136: Scope count assertions tightened; looks good.

Also applies to: 3154-3155

5088-5090: Comment-only change ok.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

.env (1)
72-73: Fix alphabetical key ordering to pass dotenv-linter checks.

The SMTP configuration keys need to be reordered alphabetically. According to static analysis hints, _APP_SMTP_PASSWORD should precede _APP_SMTP_PORT, and _APP_SMTP_USERNAME should follow _APP_SMTP_SECURE. This ordering aligns with the apparent alphabetical convention used in the file.

Apply this reordering to the SMTP configuration block (lines 69–73):
 _APP_SMTP_HOST=maildev
+_APP_SMTP_PASSWORD=password
 _APP_SMTP_PORT=1025
 _APP_SMTP_SECURE=
-_APP_SMTP_USERNAME=user
-_APP_SMTP_PASSWORD=password
+_APP_SMTP_USERNAME=user
This ensures the keys follow alphabetical order: HOST → PASSWORD → PORT → SECURE → USERNAME.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d451738 and ddb0d5d.

📒 Files selected for processing (2)

.env (1 hunks)
docker-compose.yml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

docker-compose.yml

🧰 Additional context used

🪛 dotenv-linter (4.0.0)

.env

[warning] 72-72: [UnorderedKey] The _APP_SMTP_USERNAME key should go before the _APP_STORAGE_ANTIVIRUS key

(UnorderedKey)

[warning] 73-73: [UnorderedKey] The _APP_SMTP_PASSWORD key should go before the _APP_SMTP_PORT key

(UnorderedKey)

⏰ 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). (3)

GitHub Check: Setup & Build Appwrite Image
GitHub Check: Setup & Build Appwrite Image
GitHub Check: scan

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

.env (1)
72-73: Consider reordering SMTP keys for alphabetical consistency.

The newly added _APP_SMTP_USERNAME and _APP_SMTP_PASSWORD keys break alphabetical ordering within the SMTP configuration section. For consistency with project conventions, consider reordering them as _APP_SMTP_HOST, _APP_SMTP_PASSWORD, _APP_SMTP_PORT, _APP_SMTP_SECURE, _APP_SMTP_USERNAME.
🔎 Proposed fix for alphabetical ordering
 _APP_SMTP_HOST=maildev
+_APP_SMTP_PASSWORD=password
 _APP_SMTP_PORT=1025
 _APP_SMTP_SECURE=
-_APP_SMTP_USERNAME=user
-_APP_SMTP_PASSWORD=password
+_APP_SMTP_USERNAME=user

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ddb0d5d and 36519c1.

📒 Files selected for processing (2)

.env (1 hunks)
docker-compose.yml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

docker-compose.yml

🧰 Additional context used

🪛 dotenv-linter (4.0.0)

.env

[warning] 72-72: [UnorderedKey] The _APP_SMTP_USERNAME key should go before the _APP_STORAGE_ANTIVIRUS key

(UnorderedKey)

[warning] 73-73: [UnorderedKey] The _APP_SMTP_PASSWORD key should go before the _APP_SMTP_PORT key

(UnorderedKey)

⏰ 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). (3)

GitHub Check: Setup & Build Appwrite Image
GitHub Check: scan
GitHub Check: Setup & Build Appwrite Image

VijaykumarPujar-tech and others added 7 commits December 8, 2025 20:22

Fix: robust SMTP validation and added regression test

a14d5c5

Merge branch 'main' into new_branch

0f84f21

Reverted some added changes

2fa95b5

Added the projects.php changes back

8951a84

Added Validation check for username and password

e0a9379

Merge pull request #10863 from VijaykumarPujar-tech/new_branch

3d0aa74

Fix: Implement mandatory authentication check for Custom SMTP configuration

fix: update SMTP configuration and enhance validation checks

d174233

stnguyen90 linked an issue Dec 12, 2025 that may be closed by this pull request

🐛 Bug Report: SMTP username and password aren't validated when updating SMTP settings #9067

Closed

2 tasks

Merge branch '1.8.x' into fix-smtp-auth-check

9518764

stnguyen90 requested a review from ChiragAgg5k December 12, 2025 05:44

Copilot AI mentioned this pull request Dec 12, 2025

[WIP] Fix SMTP auth check for invalid credentials #10940

Closed

3 tasks

stnguyen90 added 2 commits December 12, 2025 06:02

fix: enhance SMTP authentication by using env vars

a94ccdb

fix: update project tests to reflect changes in project count and names

d451738

stnguyen90 marked this pull request as ready for review December 12, 2025 07:44

coderabbitai bot reviewed Dec 12, 2025

View reviewed changes

ChiragAgg5k approved these changes Dec 16, 2025

View reviewed changes

Merge branch '1.8.x' into fix-smtp-auth-check

ddb0d5d

coderabbitai bot reviewed Dec 16, 2025

View reviewed changes

Merge branch '1.8.x' into fix-smtp-auth-check

36519c1

coderabbitai bot reviewed Dec 19, 2025

View reviewed changes

stnguyen90 merged commit 06e3a14 into 1.8.x Dec 19, 2025
70 of 72 checks passed

stnguyen90 deleted the fix-smtp-auth-check branch December 19, 2025 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix smtp auth check #10939

Fix smtp auth check #10939

stnguyen90 commented Dec 12, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 12, 2025 •

edited

Loading

Walkthrough

Estimated code review effort

Uh oh!

github-actions bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

stnguyen90 commented Dec 12, 2025

Uh oh!

Copilot AI commented Dec 12, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix smtp auth check #10939

Fix smtp auth check #10939

Conversation

stnguyen90 commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Uh oh!

coderabbitai bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Scan Results for PR

Docker Image Scan Results

Source Code Scan Results

Uh oh!

github-actions bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Benchmark results

⚡ Benchmark Comparison

Uh oh!

stnguyen90 commented Dec 12, 2025

Uh oh!

Copilot AI commented Dec 12, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stnguyen90 commented Dec 12, 2025 •

edited

Loading

coderabbitai bot commented Dec 12, 2025 •

edited

Loading

github-actions bot commented Dec 12, 2025 •

edited

Loading

github-actions bot commented Dec 12, 2025 •

edited

Loading