Skip to content

Modernize email delivery with OAuth2 and API transports - #5454

Merged
nilsteampassnet merged 20 commits into
nilsteampassnet:developfrom
guerricv:email-sending-system-modernization
Nov 9, 2026
Merged

nilsteampassnet merged 20 commits into
nilsteampassnet:developfrom
guerricv:email-sending-system-modernization

Conversation

@guerricv

@guerricv guerricv commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Description

TeamPass email delivery currently depends on SMTP configuration, while Microsoft and Google deployments also need unattended OAuth authentication and API delivery. This PR keeps EmailService as the common entry point and PHPMailer as the MIME builder, then adds Microsoft 365 SMTP XOAUTH2, Microsoft Graph and delegated Gmail API transports alongside existing SMTP relays.

The administration page saves one complete transport profile atomically, with explicit keep/replace/clear actions for secrets. Provider credentials and tokens are encrypted with the existing instance key. Token renewal uses configuration revisions and expiring database leases; Google consent uses state, PKCE and an administrator-bound pending configuration.

New queued messages are encrypted before insertion into the existing background-task queue. Confirmed transient failures can be deferred for at most three attempts, respecting Retry-After. Ambiguous acceptance, partial delivery and interrupted sends never trigger an automatic replay. No failure falls back to Basic SMTP authentication.

Related issue

Builds on #5453, merged into develop on 9 November 2026. The PR contains only the 14 modernization commits; its prerequisite fixes are now integrated into develop. There is no combined squash.

Temporarily in draft while the checks after the #5455 rebase complete, including Scrutinizer. The prerequisite fixes are now integrated. The remaining real-provider and interface acceptance checks are documented below.

For the modernization-only changes, compare the current develop branch with this branch. No issue is automatically closed by this PR.

Type of change

  • ☑ Bug fix (the prerequisite delivery fixes are included)
  • ☑ New feature
  • ☑ Documentation
  • ☑ Translation (English and French; other catalogs remain managed through POEditor)
  • ☑ Refactor / maintenance

How has this been tested?

Rebase after #5455 (9 November 2026): current head bee559e37a1247f9d3de0e022245ef10aac70ff9 is based on develop 2b605c51d, including the merged search favorites PR #5455 and its follow-up tests. All 14 modernization patches are unchanged. The only shared file is main.functions.php, where the email queue and favorite-access changes affect separate functions; the rebase had no conflicts. The 34 focused email/favorites tests passed with 406 assertions. Fresh CI is running on this head; the PR will return to Ready for review after all checks, including Scrutinizer, pass. Previous validation results follow for context.

History refresh (9 November 2026): previous head 89255910aa82639934ff9daebc3f7fd0329507be is rebased directly onto develop 227acfb78, the merge of #5453. GitHub now shows only the 14 modernization commits. All 14 patches match their earlier versions, and the complete Git tree is identical to 63bd305fb; no application file changed. Fresh CI on this new head passed: PHP 8.2/8.3 each ran 3,636 tests and 84,619 assertions; all 319 JavaScript tests, PHPStan level 4, MariaDB, repository guards, CodeQL and Scrutinizer passed. All 11 checks are green, and the PR is ready for code review. The documented real-provider and visual acceptance checks remain open. Earlier source validation results follow for context.

Rebase follow-up (9 November 2026): previous head 63bd305fb00ac43baa7e8e50d3ff15c088c8b9b9 is based on the review fixes in #5453 (004760823), above develop 945069257. The 14 modernization commits remain separate. Conflicts were resolved to retain the connection/TLS diagnostics, safe decryption errors, failed-task anonymization and purge requested in #5453. Terminal failures replace encrypted payloads with failure metadata without claiming provider acceptance; deferred sealed messages retain their ciphertext for retry. Regression coverage includes both outcomes.

Validation before the history refresh (63bd305): Quality CI passed: PHP 8.2/8.3 each ran 3,636 tests and 84,619 assertions; all 319 JavaScript tests, PHPStan level 4, MariaDB and repository guards passed. CodeQL passed. Scrutinizer also passed (two non-blocking findings reported); all 11 checks are green on this head. The source tree passed these checks before the history refresh; the acceptance checks below remain open. Locally, 259 focused tests and 2,043 assertions passed (five skips), along with explicit PHPStan analysis of the email package, worker and handler. The isolated Linux copy passed 3,636 tests and 84,569 assertions on PHP 8.5.4 (14 deprecations and five skips), plus both email MariaDB regressions. Earlier validation results follow for context.

Rebase follow-up (8 November 2026): rebased onto the updated prerequisite #5453, itself based on develop at 2e3db0edde1b4bfb1a93c84f5944e964c278b548 (TeamPass 3.2.4.2). The three prerequisite commits and 14 modernization commits remain separate. The only conflict was the version file: the official 3.2.4.2 version and the email schema marker were both retained; the migration test comment now names the current release. Current head: e19739b618c6b749a55deceef7bb881075a9643e. Local email/background-task/upgrade tests passed (254 tests, 1,914 assertions; five environment skips), as did explicit PHPStan analysis of the email package, the table-prefix guard and dual-copy checks. Fresh Quality CI passed on this rebased head: PHP 8.2/8.3 each passed 3,619 tests and 84,429 assertions; all 311 JavaScript tests, PHPStan level 4, MariaDB and repository guards passed. CodeQL and external Scrutinizer also passed on this rebased head; all 11 checks are green. The new upstream security regressions also passed locally from an LF archive (34 tests, 455 assertions); CRLF-sensitive source assertions fail in the Windows checkout. The results below are historical checks of the earlier head. It remained a draft at that stage.

The upstream PR checks also passed on 7910412ce49e5cd0aa810f1d8120f44905436263, including PHP 8.2/8.3, JavaScript, PHPStan, MariaDB, repository guards, CodeQL and external Scrutinizer. The PR was still a draft at that stage.

Quality CI on commit 7910412ce49e5cd0aa810f1d8120f44905436263 passed all seven jobs:

  • PHP 8.2 and PHP 8.3: 3,585 tests and 83,972 assertions on each version.
  • JavaScript on Linux: all 311 tests passed.
  • PHPStan level 4, MariaDB integration, the production-autoloader guard and the SQL table-prefix guard passed.

An isolated Linux test copy also passed 3,585 tests and 83,922 assertions on PHP 8.5.4, with the same 14 deprecations and five skipped tests as the baseline, plus full PHPStan. The transport package was analyzed separately because the main PHPStan configuration excludes these library copies.

Regression coverage includes real PHPMailer MIME preparation, synthetic loopback SMTP sessions for relay/LOGIN/XOAUTH2, mocked provider HTTP responses, token cache and renewal ownership, consent binding, encrypted queue payloads, worker and handler outcomes, deferred scheduling, and the refusal to replay ambiguous results. MariaDB tests execute fresh-install and upgrade code, replay the migration, and verify encryption, rollback, configuration concurrency and due-task selection with a non-default table prefix.

Acceptance checks still pending: real Microsoft/Google credentials, mailbox permission scoping, consent and renewal against live providers, inbox receipt and client rendering, and visual review of the administration page on small screens and in dark mode. The browser automation host was unavailable. No manual admin/manager/standard/read-only role matrix or personal-folder scenario is claimed; these remain in the documented acceptance procedure. Installed application data was not used by the isolated tests.

Checklist

  • ☑ PHPStan level 4 passes.
  • ☑ The PHP and JavaScript test suites pass in Linux CI.
  • ☑ Every new public function has a docblock.
  • ☑ New variable names and comments are in English.
  • ☑ No var_dump() or console.log() was added.
  • ☑ New private controllers have matching public proxy shims, including the OAuth callback.
  • ☑ All email-service classes have synchronized source and vendor copies, covered by the sentinel test.
  • ☑ The committed Composer autoloader remains in production form.
  • ☐ Real-provider and visual acceptance checks are complete.

Impact on install / upgrade

Schema change: the replayable upgrade_run_3.2.4.php step and fresh installation add provider settings, a configuration revision, email_oauth_state, and the email_attempts/next_attempt_at task columns with a due-task index. UPGRADE_MIN_DATE is advanced so existing 3.2.4.1 and 3.2.4.2 installations also run the migration; the release number is unchanged.

Existing SMTP settings are preserved and remain the default. Historical queued messages remain readable. New encrypted task envelopes require compatible workers: restore compatible code and database backups together when rolling back, and account for pending encrypted tasks before a downgrade.

Guzzle and League OAuth2 Client were already shipped transitively; they are declared directly without changing their locked versions or rebuilding the committed production autoloader.

Initial API limits are explicit: 2 MiB of prepared MIME, matching configured sender and From, and Bcc refused rather than silently dropped. SMTP retains its existing envelope behavior. Google SMTP OAuth, service accounts/domain-wide delegation, national-cloud endpoints, Microsoft certificate credentials and native transactional-provider APIs are outside this version.

Administration, provider permissions, migration, rollback, limitations and acceptance steps are documented in docs/manage/email-transports.md. Transport and queue design notes are in .claude/rules/architecture-email-transports.md.

Screenshots

Pending visual acceptance. Server-rendered controls were checked for secret leakage and escaped labels, and the generated JavaScript syntax was verified; these checks do not replace a browser review.

image image image

@guerricv
guerricv force-pushed the email-sending-system-modernization branch 2 times, most recently from e19739b to 63bd305 Compare November 9, 2026 06:51
@guerricv
guerricv marked this pull request as ready for review November 9, 2026 07:06
@guerricv
guerricv marked this pull request as draft November 9, 2026 08:53
@guerricv
guerricv force-pushed the email-sending-system-modernization branch from 63bd305 to 8925591 Compare November 9, 2026 08:53
@guerricv
guerricv marked this pull request as ready for review November 9, 2026 09:10
@guerricv
guerricv marked this pull request as draft November 9, 2026 09:27
@guerricv
guerricv force-pushed the email-sending-system-modernization branch from 8925591 to bee559e Compare November 9, 2026 09:27
@nilsteampassnet

nilsteampassnet commented Nov 9, 2026 •

Copy link
Copy Markdown
Owner

Thanks @guerricv.

Two points must be fixed before merge:

  1. UPGRADE_MIN_DATE is older than the 3.2.4.2 release. 1794009600 is 2026-11-07 00:00 UTC, but 3.2.4.2 was tagged 2026-11-08 08:28 UTC (1794126505). upgrade_timestamp is time() at install/upgrade, so every fresh 3.2.4.2 install and every full upgrade run since then has a higher timestamp: neither upgradeRequired() nor tpUpgradeCliDecide() replays the migration. The handler guard then lets the SELECT on next_attempt_at through and all background tasks stop on the missing column. EmailUpgradeRequirementTest passes only because it uses an invented 26 Oct timestamp. Please make the test assert UPGRADE_MIN_DATE > 1794126505 (3.2.4.2 release). I will set the final value myself at merge time.
  2. The Tasks log loses the recipient. Completion and terminal-failure arguments are now {"accepted":true} / {"failed":true}, so the user column of Logs → Tasks (resolveBackgroundTaskUserDisplay(), logs.datatables.php:110) goes blank for every email. Fix email delivery isolation and background failure reporting #5453 deliberately kept email + login, and your new message asks the admin to "check the provider before resending", which they can't do without knowing who the email was for. Please keep email and login (never subject, body or encryptedUserPassword) in both completion and failure metadata, captured in sendEmail() right after emailQueueOpen().

Strongly suggested in the same round:

  1. Apply proxy_ip / proxy_port in EmailHttp::options() (Guzzle proxy), as licence.functions.php:249 does. Corporate Microsoft 365 installs are usually behind one, and token acquisition for SMTP XOAUTH2 goes through it too.
  2. Cap the provider Retry-After in emailQueueRetryAt() (for example 1 h) and fail beyond it, instead of parking a task for years.
  3. When every RCPT is refused, PHPMailer never sends DATA (PHPMailer.php:2157), so with one recipient per message that is a definite rejected, not partial_acceptance.

Smaller points:

  • Log the exception class (not the message) in the final catch (\Throwable) of sendMail().
  • The SMTP host validation now refuses PHPMailer host lists (a;b) and tls:// prefixes. Either accept them or say it in the docs.
  • Consider moving email_debug_level out of the transport profile: with Gmail active, changing it forces a new Google consent.

Real-provider and visual acceptance remain on your list, as you noted. I'll handle POEditor for the other languages.

@guerricv

guerricv commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks Nils, both blockers were valid, and I've addressed the other suggestions too. The fixes are split into six commits:

  • 839a96429: the migration floor is now newer than the shipped 3.2.4.2 release. The regression asserts UPGRADE_MIN_DATE > 1794126505 and uses an installation timestamp after that release. The new value is provisional for you to refresh at merge time.
  • ac1d6a077: sendEmail() captures email and login immediately after emailQueueOpen(). Completed and terminally failed tasks retain those fields and their outcome only; deferred tasks retain the sealed payload. The regressions execute the actual Tasks log resolver, including its email fallback, without reopening the payload for logging.
  • 223ab524d: provider HTTP and token acquisition use the global proxy_ip / proxy_port, including the SMTP XOAUTH2 token request. TLS verification stays enabled.
  • 4c3e30221: Retry-After is honored up to one hour. Longer delays fail the task rather than parking it indefinitely or retrying too early.
  • 83a9f7172: all RCPT refusals are rejected; partial acceptance after DATA remains indeterminate and cannot replay automatically. Real loopback SMTP tests cover one/all recipients refused and partial acceptance without a recognized queue ID. Unexpected exceptions log their class only. I also documented the editor's single-host restriction and the refusal of host lists and tls:// / ssl:// prefixes; existing stored settings remain readable.
  • 2ff23f61e: debug level autosaves independently, without rotating the transport revision, touching tokens or starting Gmail consent. The server validates the level; PHP and JavaScript regressions cover the separate save paths.

Local validation: 258 focused PHP tests / 2,576 assertions, explicit PHPStan analysis of the email package and worker/queue/profile code, PHP syntax and table-prefix checks all passed. Both new JavaScript regressions passed too.

Fresh Quality and CodeQL checks passed on 2ff23f61e: PHP 8.2 and 8.3 each ran 3,656 tests / 85,048 assertions; all 336 JavaScript tests, PHPStan level 4, MariaDB integration and repository guards passed. Scrutinizer has not created a new inspection or attached a status to this head yet; its previous green result belongs to bee559e37 and does not validate these fixes. The PR is now ready for review with all ten GitHub checks green. Scrutinizer remains outstanding: we have not been able to trigger a manual rerun, and no quota warning is shown in its dashboard. This missing external check is not being treated as a successful analysis.

Live Microsoft/Google permission, consent/renewal, inbox delivery and visual acceptance remain open as documented. No installed application data was used for these tests.

@guerricv
guerricv marked this pull request as ready for review November 9, 2026 10:29
@nilsteampassnet
nilsteampassnet merged commit fe68747 into nilsteampassnet:develop Nov 9, 2026
11 checks passed
@nilsteampassnet

Copy link
Copy Markdown
Owner

Thanks @guerricv, #5454 is merged and pushed to develop. A post-merge review found two gaps that must be fixed before the next release. Please open one follow-up PR against develop that covers the points below.

Must fix

  1. "Email is configured" still means email_smtp_server !== ''. With Microsoft Graph, Gmail API or SMTP microsoft_oauth, the SMTP host field is hidden and stays empty on a fresh configuration. TeamPass then behaves as if email were not configured:

    • getForgotLocalPasswordContext() (app/sources/main.functions.php:4454): local password recovery is silently no longer offered;
    • webauthnLoginNotifyAdded() (app/sources/webauthn_login.functions.php:1111) and app/api/Model/WebauthnModel.php:548: the "passkey added" security email is skipped;
    • tpUpgradeCliNotify() (app/sources/upgrade_cli.functions.php:837): no upgrade notification is sent;
    • app/pages/users.php:227: a false "SMTP server not configured" warning is shown;
    • adminNoticesSmtpCertificateUnverified() (app/sources/admin_notices_logic.php:214): can warn about an SMTP relay that is no longer used.

    Please add one DB-free helper, for example emailTransportIsConfigured(array $settings): bool in emails_transport_logic.php, loaded from main.functions.php next to emails_queue_logic.php (every caller above, the API included, already loads main.functions.php). Suggested rules:

    • smtp + none/password: non-empty email_smtp_server;
    • smtp + microsoft_oauth, or microsoft_graph: valid email_microsoft_mailbox;
    • gmail_api: valid email_google_sender;
    • an empty auth mode is derived from email_smtp_auth, as EmailSettings does.

    Use it at all five call sites. The certificate notice should only apply to SMTP with none/password, since microsoft_oauth always verifies TLS. WebauthnNotificationAndAuditTest.php:53 asserts the literal 'email_smtp_server' and needs updating. Please add a unit test that covers each transport.

  2. The bug report exposes the new provider settings. generateBugReport() (app/sources/main.queries.php:2078, $config_exclude_vars) masks oauth2_client_id, oauth2_tenant_id, oauth2_client_secret and email_from. It does not mask the seven new keys: email_microsoft_tenant_id, email_microsoft_client_id, email_microsoft_client_secret, email_microsoft_mailbox, email_google_client_id, email_google_client_secret, email_google_sender. This report is meant to be pasted into a public GitHub issue. Please add these keys, and also merge tpSecretSettingNames() into the list so any future secret is masked by default. A sentinel test on that list would prevent this from happening again.

Strongly suggested in the same PR

  1. Profile save errors are opaque. emailTransportCandidate() throws the same InvalidArgumentException('configuration') for every rule, and the handler always answers email_profile_save_failed. Existing installs that store tls://host or h1;h2 can no longer save any change to the profile, and they get no hint why. Please return the name of the offending field, taken from the closed emailTransportSettingNames() list (never a value), and have the page show which field is wrong.
  2. prepareSendingEmail() can now throw. emailQueueSeal() and JSON_THROW_ON_ERROR (main.functions.php:2337) can raise an exception, and none of its 15 callers catch it. Two of them run during sign-in: the 2FA code email at identify.php:431 and the login notification at identify.php:1484. An encryption failure there aborts the login for what may be only a notification. Please catch the exception inside the function, log only the exception class, and return false (: bool). Current callers ignore the return value, and callers that care can check it.
  3. The upgrade step breaks the web wizard's error format. The six throw new RuntimeException() calls in upgrade_run_3.2.4.php make the web upgrade return an HTTP 500 instead of the [{"finish":"1","error":"…"}] answer the wizard parses (see line 75 of the same file). Please echo that format and exit; upgrade-cli.php already catches Throwable.

Smaller point

  • An invalid proxy_port makes EmailHttp::options() throw InvalidArgumentException. requestToken() (EmailTokenManager.php:185) turns it into a retryable network failure, and ApiTransport::send() (line 94) into an indeterminate one. It is a configuration error: please map it to configuration, with no retry.

Your real-provider and visual acceptance checks are still pending, as you noted. I'll handle POEditor and the release note (encrypted queue payloads and rollback, SMTP host formats now refused).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants